[tint] Remove TODO(bclayton)
Replace with a bug, or remove if I've decided this isn't worth doing.
Also: s/Permuter/Permutator
Change-Id: If53a900778dc31f133c32758aa6175aaeb21cd6f
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/183220
Commit-Queue: dan sinclair <dsinclair@chromium.org>
Auto-Submit: Ben Clayton <bclayton@google.com>
Reviewed-by: dan sinclair <dsinclair@chromium.org>
diff --git a/src/tint/CMakeLists.txt b/src/tint/CMakeLists.txt
index 88fa5f8..da7fda4 100644
--- a/src/tint/CMakeLists.txt
+++ b/src/tint/CMakeLists.txt
@@ -269,7 +269,7 @@
target_link_libraries(${TARGET} PRIVATE "tint_api_sanitize_fuzzer")
endfunction()
-# TODO(bclayton): Remove this when fuzzers fully migrated to gen build
+# TODO(crbug.com/tint/2223): Remove this when fuzzers are fully migrated to gen build
function(tint_fuzzer_compile_options TARGET)
tint_fuzz_cmd_compile_options(${TARGET})
target_link_libraries(${TARGET} PRIVATE "tint_api_sanitize_fuzzer")
diff --git a/src/tint/lang/core/constant/eval.h b/src/tint/lang/core/constant/eval.h
index f3c4e6e..9efe390 100644
--- a/src/tint/lang/core/constant/eval.h
+++ b/src/tint/lang/core/constant/eval.h
@@ -55,8 +55,6 @@
class Eval {
public:
/// The failure type used by the methods of this class.
- /// TODO(bclayton): Use Failure, and bubble up the error diagnostics instead of writing directly
- /// to the diagnostic list.
struct Error {};
/// A value of the type Error.
diff --git a/src/tint/lang/core/constant/eval_test.h b/src/tint/lang/core/constant/eval_test.h
index 54aa26e..2158453 100644
--- a/src/tint/lang/core/constant/eval_test.h
+++ b/src/tint/lang/core/constant/eval_test.h
@@ -42,13 +42,13 @@
namespace tint::core::constant::test {
-// TODO(bclayton): Don't depend on resolver
+// TODO(crbug.com/tint/2221): Don't depend on resolver
namespace builder = tint::resolver::builder;
-// TODO(bclayton): Don't depend on resolver
+// TODO(crbug.com/tint/2221): Don't depend on resolver
using ConstEvalTest = resolver::ResolverTest;
-// TODO(bclayton): Don't depend on resolver
+// TODO(crbug.com/tint/2221): Don't depend on resolver
template <typename T>
using ConstEvalTestWithParam = resolver::ResolverTestWithParam<T>;
diff --git a/src/tint/lang/core/constant/manager.h b/src/tint/lang/core/constant/manager.h
index 993a388..2eb5088 100644
--- a/src/tint/lang/core/constant/manager.h
+++ b/src/tint/lang/core/constant/manager.h
@@ -69,8 +69,8 @@
/// The Manager returned by Wrap is intended to temporarily extend the constants and types of an
/// existing immutable Manager. As the copied constants and types are owned by `inner`, `inner`
/// must not be destructed or assigned while using the returned Manager.
- /// TODO(bclayton) - Evaluate whether there are safer alternatives to this
- /// function. See crbug.com/tint/460.
+ /// TODO(crbug.com/tint/460) - Evaluate whether there are safer alternatives to this
+ /// function.
/// @param inner the immutable Manager to extend
/// @return the Manager that wraps `inner`
static Manager Wrap(const Manager& inner) {
diff --git a/src/tint/lang/core/type/manager.h b/src/tint/lang/core/type/manager.h
index 63052af..74331f4 100644
--- a/src/tint/lang/core/type/manager.h
+++ b/src/tint/lang/core/type/manager.h
@@ -111,8 +111,8 @@
/// of an existing immutable Manager.
/// @warning As the copied types are owned by `inner`, `inner` must not be destructed or
/// assigned while using the returned Manager.
- /// TODO(bclayton) - Evaluate whether there are safer alternatives to this
- /// function. See crbug.com/tint/460.
+ /// TODO(crbug.com/tint/460) - Evaluate whether there are safer alternatives to this
+ /// function.
/// @param inner the immutable Manager to extend
/// @return the Manager that wraps `inner`
static Manager Wrap(const Manager& inner) {
diff --git a/src/tint/lang/spirv/reader/ast_parser/parse.cc b/src/tint/lang/spirv/reader/ast_parser/parse.cc
index c3dea0e..ac50d1e 100644
--- a/src/tint/lang/spirv/reader/ast_parser/parse.cc
+++ b/src/tint/lang/spirv/reader/ast_parser/parse.cc
@@ -82,7 +82,6 @@
ProgramBuilder& builder = parser.builder();
if (!parsed) {
- // TODO(bclayton): Migrate ASTParser to using diagnostics.
builder.Diagnostics().AddError(diag::System::Reader, Source{}) << parser.error();
return Program(std::move(builder));
}
diff --git a/src/tint/lang/spirv/writer/ast_raise/var_for_dynamic_index.cc b/src/tint/lang/spirv/writer/ast_raise/var_for_dynamic_index.cc
index 03e6498..8dd04c4 100644
--- a/src/tint/lang/spirv/writer/ast_raise/var_for_dynamic_index.cc
+++ b/src/tint/lang/spirv/writer/ast_raise/var_for_dynamic_index.cc
@@ -71,8 +71,6 @@
return true;
}
- // TODO(bclayton): group multiple accesses in the same object.
- // e.g. arr[i] + arr[i+1] // Don't create two vars for this
return hoist_to_decl_before.Add(indexed, object_expr,
ast::transform::HoistToDeclBefore::VariableKind::kVar,
"var_for_index");
diff --git a/src/tint/lang/spirv/writer/ast_raise/var_for_dynamic_index_test.cc b/src/tint/lang/spirv/writer/ast_raise/var_for_dynamic_index_test.cc
index 5d95703..84640ce 100644
--- a/src/tint/lang/spirv/writer/ast_raise/var_for_dynamic_index_test.cc
+++ b/src/tint/lang/spirv/writer/ast_raise/var_for_dynamic_index_test.cc
@@ -102,7 +102,6 @@
}
)";
- // TODO(bclayton): Optimize this case:
// This output is not as efficient as it could be.
// We only actually need to hoist the inner-most array to a `var`
// (`var_for_index`), as later indexing operations will be working with
diff --git a/src/tint/lang/wgsl/ast/clone_context.h b/src/tint/lang/wgsl/ast/clone_context.h
index b99f774..07d5de7 100644
--- a/src/tint/lang/wgsl/ast/clone_context.h
+++ b/src/tint/lang/wgsl/ast/clone_context.h
@@ -127,9 +127,7 @@
ast::Type Clone(const ast::Type& ty);
/// Clones the Source `s` into #dst
- /// TODO(bclayton) - Currently this 'clone' is a shallow copy. If/when
- /// `Source.File`s are owned by the Program this should make a copy of the
- /// file.
+ /// @note this 'clone' is a shallow copy.
/// @param s the `Source` to clone
/// @return the cloned source
Source Clone(const Source& s) const { return s; }
diff --git a/src/tint/lang/wgsl/ls/definition_test.cc b/src/tint/lang/wgsl/ls/definition_test.cc
index f9d9780..df026ec 100644
--- a/src/tint/lang/wgsl/ls/definition_test.cc
+++ b/src/tint/lang/wgsl/ls/definition_test.cc
@@ -74,7 +74,7 @@
}
}
-// TODO(bclayton): Type aliases.
+// TODO(crbug.com/tint/2127): Type aliases.
INSTANTIATE_TEST_SUITE_P(,
LsDefinitionTest,
::testing::ValuesIn(std::vector<std::string_view>{
diff --git a/src/tint/lang/wgsl/ls/file.h b/src/tint/lang/wgsl/ls/file.h
index 90a86de..e860eb9 100644
--- a/src/tint/lang/wgsl/ls/file.h
+++ b/src/tint/lang/wgsl/ls/file.h
@@ -99,7 +99,8 @@
/// @tparam T the type or subtype of the node to scan for.
template <typename T = sem::Node, UnwrapMode UNWRAP_MODE = DefaultUnwrapMode<T>>
const T* NodeAt(Source::Location l) const {
- // TODO(bclayton): This is a brute-force search. Optimize.
+ // TODO(crbug.com/tint/2127): This is a brute-force search. Optimize.
+ // Suggested optimization: bin the intersecting ranges per-line.
size_t best_len = std::numeric_limits<size_t>::max();
const T* best_node = nullptr;
for (auto* node : nodes) {
diff --git a/src/tint/lang/wgsl/ls/hover_test.cc b/src/tint/lang/wgsl/ls/hover_test.cc
index 048d8da..576be7c 100644
--- a/src/tint/lang/wgsl/ls/hover_test.cc
+++ b/src/tint/lang/wgsl/ls/hover_test.cc
@@ -85,7 +85,7 @@
}
}
-// TODO(bclayton): Type aliases.
+// TODO(crbug.com/tint/2127): Type aliases.
INSTANTIATE_TEST_SUITE_P(
,
LsHoverTest,
diff --git a/src/tint/lang/wgsl/ls/references_test.cc b/src/tint/lang/wgsl/ls/references_test.cc
index df732ec..5693cfe 100644
--- a/src/tint/lang/wgsl/ls/references_test.cc
+++ b/src/tint/lang/wgsl/ls/references_test.cc
@@ -84,7 +84,7 @@
}
}
-// TODO(bclayton): Type aliases.
+// TODO(crbug.com/tint/2127): Type aliases.
INSTANTIATE_TEST_SUITE_P(IncludeDeclaration,
LsReferencesTest,
::testing::ValuesIn(std::vector<Case>{
diff --git a/src/tint/lang/wgsl/ls/rename_test.cc b/src/tint/lang/wgsl/ls/rename_test.cc
index 8e16670..7979e60 100644
--- a/src/tint/lang/wgsl/ls/rename_test.cc
+++ b/src/tint/lang/wgsl/ls/rename_test.cc
@@ -133,7 +133,7 @@
}
}
-// TODO(bclayton): Type aliases.
+// TODO(crbug.com/tint/2127): Type aliases.
INSTANTIATE_TEST_SUITE_P(,
LsRenameTest,
::testing::ValuesIn(std::vector<Case>{
diff --git a/src/tint/lang/wgsl/ls/sem_tokens_test.cc b/src/tint/lang/wgsl/ls/sem_tokens_test.cc
index d7fc9a0..a9cc2de 100644
--- a/src/tint/lang/wgsl/ls/sem_tokens_test.cc
+++ b/src/tint/lang/wgsl/ls/sem_tokens_test.cc
@@ -118,7 +118,7 @@
}
}
-// TODO(bclayton): Type aliases.
+// TODO(crbug.com/tint/2127): Type aliases.
INSTANTIATE_TEST_SUITE_P(,
LsSemTokensTest,
::testing::ValuesIn(std::vector<Case>{
diff --git a/src/tint/lang/wgsl/ls/symbols.cc b/src/tint/lang/wgsl/ls/symbols.cc
index e1db1ab..03bd2be 100644
--- a/src/tint/lang/wgsl/ls/symbols.cc
+++ b/src/tint/lang/wgsl/ls/symbols.cc
@@ -77,7 +77,7 @@
lsp::DocumentSymbol sym;
sym.range = (*file)->Conv(str->source.range);
sym.selection_range = (*file)->Conv(decl->name->source.range);
- // TODO(bclayton): Is there a better symbol kind?
+ // TODO(crbug.com/tint/2127): Is there a better symbol kind?
sym.kind = lsp::SymbolKind::kObject;
sym.name = decl->name->symbol.NameView();
symbols.push_back(sym);
diff --git a/src/tint/lang/wgsl/program/program_builder.h b/src/tint/lang/wgsl/program/program_builder.h
index 2e2bb49..ba9980e 100644
--- a/src/tint/lang/wgsl/program/program_builder.h
+++ b/src/tint/lang/wgsl/program/program_builder.h
@@ -99,8 +99,8 @@
/// existing immutable program.
/// As the returned ProgramBuilder wraps `program`, `program` must not be
/// destructed or assigned while using the returned ProgramBuilder.
- /// TODO(bclayton) - Evaluate whether there are safer alternatives to this
- /// function. See crbug.com/tint/460.
+ /// TODO(crbug.com/tint/460) - Evaluate whether there are safer alternatives to this
+ /// function.
/// @param program the immutable Program to wrap
/// @return the ProgramBuilder that wraps `program`
static ProgramBuilder Wrap(const Program& program);
diff --git a/src/tint/lang/wgsl/reader/parser/parser.cc b/src/tint/lang/wgsl/reader/parser/parser.cc
index d4ba3ca..e9eee00 100644
--- a/src/tint/lang/wgsl/reader/parser/parser.cc
+++ b/src/tint/lang/wgsl/reader/parser/parser.cc
@@ -1442,17 +1442,12 @@
return Failure::kNoMatch;
}
- if (peek_is(Token::Type::kSemicolon)) {
- return builder_.Return(source, nullptr);
- }
-
auto expr = expression();
if (expr.errored) {
return Failure::kErrored;
}
- // TODO(bclayton): Check matched?
- return builder_.Return(source, expr.value);
+ return expr.matched ? builder_.Return(source, expr.value) : builder_.Return(source);
}
// variable_statement
diff --git a/src/tint/lang/wgsl/writer/raise/rename_conflicts.cc b/src/tint/lang/wgsl/writer/raise/rename_conflicts.cc
index c86f3b1..f40dd88 100644
--- a/src/tint/lang/wgsl/writer/raise/rename_conflicts.cc
+++ b/src/tint/lang/wgsl/writer/raise/rename_conflicts.cc
@@ -281,8 +281,7 @@
/// @return true if @p s is a builtin (non-user declared) structure.
bool IsBuiltinStruct(const core::type::Struct* s) {
- // TODO(bclayton): Need to do better than this.
- return tint::HasPrefix(s->Name().NameView(), "_");
+ return tint::HasPrefix(s->Name().NameView(), "__");
}
};
diff --git a/src/tint/tint.gni b/src/tint/tint.gni
index 8ae6c91..b284b4b 100644
--- a/src/tint/tint.gni
+++ b/src/tint/tint.gni
@@ -171,7 +171,8 @@
if (target_name == "wgsl") {
dict = "dictionary.txt"
libfuzzer_options = [
- "only_ascii=1", # TODO(bclayton): Remove this to fuzz unicode?
+ # TODO(crbug.com/tint/2223): Remove this to fuzz unicode?
+ "only_ascii=1",
"max_len=10000",
]
seed_corpus = fuzzer_corpus_wgsl_dir
diff --git a/src/tint/utils/generator/text_generator.cc b/src/tint/utils/generator/text_generator.cc
index 0bcab39..e7140cb 100644
--- a/src/tint/utils/generator/text_generator.cc
+++ b/src/tint/utils/generator/text_generator.cc
@@ -80,7 +80,7 @@
void TextGenerator::TextBuffer::Append(const TextBuffer& tb) {
for (auto& line : tb.lines) {
- // TODO(bclayton): inefficient, consider optimizing
+ // TODO(crbug.com/tint/2222): inefficient, consider optimizing
lines.emplace_back(LineInfo{current_indent + line.indent, line.content});
}
}
@@ -94,7 +94,7 @@
}
size_t idx = 0;
for (auto& line : tb.lines) {
- // TODO(bclayton): inefficient, consider optimizing
+ // TODO(crbug.com/tint/2222): inefficient, consider optimizing
using DT = decltype(lines)::difference_type;
lines.insert(lines.begin() + static_cast<DT>(before + idx),
LineInfo{indent + line.indent, line.content});
diff --git a/src/tint/utils/id/generation_id.h b/src/tint/utils/id/generation_id.h
index 8902a53..018b4a2 100644
--- a/src/tint/utils/id/generation_id.h
+++ b/src/tint/utils/id/generation_id.h
@@ -37,10 +37,8 @@
namespace tint {
-/// If 1 then checks are enabled that AST nodes are not leaked from one generation
-/// to another.
-/// TODO(bclayton): We'll want to disable this in production builds. For now we
-/// always check.
+/// If 1 then checks are enabled that AST nodes are not leaked from one generation to another.
+/// It may be worth disabling this in production builds. For now we always check.
#define TINT_CHECK_FOR_CROSS_GENERATION_LEAKS 1
/// A GenerationID is a unique identifier of a generation.
diff --git a/tools/src/cmd/gen/templates/templates.go b/tools/src/cmd/gen/templates/templates.go
index d8191c8..5dd5b16 100644
--- a/tools/src/cmd/gen/templates/templates.go
+++ b/tools/src/cmd/gen/templates/templates.go
@@ -189,7 +189,7 @@
path string
cachedSem *sem.Sem // lazily built by sem()
cachedTable *gen.IntrinsicTable // lazily built by intrinsicTable()
- cachedPermuter *gen.Permuter // lazily built by permute()
+ cachedPermuter *gen.Permutator // lazily built by permute()
}
// Sem lazily parses and resolves the intrinsic.def file, returning the semantic info.
@@ -244,7 +244,7 @@
if err != nil {
return nil, err
}
- i.cachedPermuter, err = gen.NewPermuter(sem)
+ i.cachedPermuter, err = gen.NewPermutator(sem)
if err != nil {
return nil, err
}
diff --git a/tools/src/cmd/run-cts/chrome/cmd.go b/tools/src/cmd/run-cts/chrome/cmd.go
index 7a62f60..c447b4e 100644
--- a/tools/src/cmd/run-cts/chrome/cmd.go
+++ b/tools/src/cmd/run-cts/chrome/cmd.go
@@ -288,7 +288,7 @@
requests <- Request{Query: string(res.TestCase)}
for {
- // TODO(bclayton): Implement timeouts, browser restarting.
+ // Future enhancements: Timeouts, browser restarting.
select {
case response := <-responses:
switch response.Type {
diff --git a/tools/src/tint/intrinsic/gen/permutate.go b/tools/src/tint/intrinsic/gen/permutate.go
index 4a79968..cdd5a55 100644
--- a/tools/src/tint/intrinsic/gen/permutate.go
+++ b/tools/src/tint/intrinsic/gen/permutate.go
@@ -37,25 +37,26 @@
"dawn.googlesource.com/dawn/tools/src/tint/intrinsic/sem"
)
-// Permuter generates permutations of intrinsic overloads
-type Permuter struct {
+// Permutator generates permutations of intrinsic overloads
+type Permutator struct {
sem *sem.Sem
allTypes []sem.FullyQualifiedName
}
-// NewPermuter returns a new initialized Permuter
-func NewPermuter(s *sem.Sem) (*Permuter, error) {
+// NewPermutator returns a new initialized Permutator
+func NewPermutator(s *sem.Sem) (*Permutator, error) {
// allTypes are the list of FQNs that are used for unconstrained types
allTypes := []sem.FullyQualifiedName{}
for _, ty := range s.Types {
if len(ty.TemplateParams) > 0 {
// Ignore aggregate types for now.
- // TODO(bclayton): Support a limited set of aggregate types
+ // If generation of e2e tests with aggregate types is required then selectively allow
+ // them here - just be careful with permutation explosions!
continue
}
allTypes = append(allTypes, sem.FullyQualifiedName{Target: ty})
}
- return &Permuter{
+ return &Permutator{
sem: s,
allTypes: allTypes,
}, nil
@@ -70,9 +71,9 @@
}
// Permute generates a set of permutations for the given intrinsic overload
-func (p *Permuter) Permute(overload *sem.Overload) ([]Permutation, error) {
+func (p *Permutator) Permute(overload *sem.Overload) ([]Permutation, error) {
state := permutationState{
- Permuter: p,
+ Permutator: p,
templateTypes: map[sem.TemplateParam]sem.FullyQualifiedName{},
templateNumbers: map[sem.TemplateParam]any{},
parameters: map[int]sem.FullyQualifiedName{},
@@ -240,7 +241,7 @@
}
type permutationState struct {
- *Permuter
+ *Permutator
templateTypes map[sem.TemplateParam]sem.FullyQualifiedName
templateNumbers map[sem.TemplateParam]any
parameters map[int]sem.FullyQualifiedName
diff --git a/tools/src/tint/intrinsic/resolver/resolve.go b/tools/src/tint/intrinsic/resolver/resolve.go
index abd5e44..b617082 100644
--- a/tools/src/tint/intrinsic/resolver/resolve.go
+++ b/tools/src/tint/intrinsic/resolver/resolve.go
@@ -678,8 +678,6 @@
anyNumber := "any number"
// asNumber() returns anyNumber if n is a TemplateNumberParam.
- // TODO(bclayton): Once we support number ranges [e.g.: fn F<N: 1..4>()], we
- // should check number ranges are compatible
asNumber := func(n sem.Named) interface{} {
switch n.(type) {
case *sem.TemplateNumberParam:
