hokein added a comment.
Just share my experience when reading the code, overall I have a feeling that
we're regressing the code readability of the tests (left some comments on
regressing samples):
1. with the sugared macros, the syntax of `TargetCode` and `ReferenceCode` are
identical -- both using the `^` without any names, it is hard to distinguish
them (we could use a different syntax `[[]]` in the ReferenceCode to identify
the point);
2. the code of using `EXPECT_REFERENCES_WITH_TYPES` is not easy to read
(especially with multiple `RefType` arguments), we're losing the relationship
between RefType and `^` point, this is my biggest concern;
3. the `EXPECT_{EXPLICIT, IMPLICIT, EXPLICIT}_REFERENCES` is designed for a
single reference point. When we have > 1 reference points, we have to use the
fallback `EXPECT_REFERENCES_WITH_TYPES` even if all ref points are the same
type (not sure it is generic enough, maybe it is fine as single-ref-point is
the most common case);
That's being said, making the tests less verbose is good, but we seem to trade
too much code readability for that. I'm fine with the original annotation
version even though the annotations are somewhat noisy, another alternative --
if we use abbreviations like (`$E`, `$I`, `$A`) in tests, would it be better?
================
Comment at: clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp:156
namespace ns {
- void $ambiguous^x(); void $ambiguous^x(int); void $ambiguous^x(char);
})cpp",
----------------
a bug in existing code? by looking at this example, we should use
`EXPECT_AMBIGUOUS_REFERENCES(Code, Code, RefType::Ambiguous,
RefType::Ambiguous, RefType::Ambiguous)`
================
Comment at: clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp:234
+ "// c++-header\n using ns::^x; void foo() { x(3); }", RefType::Ambiguous,
+ RefType::Explicit, RefType::Ambiguous);
+ EXPECT_EXPLICIT_REFERENCES("namespace ns { struct S; } using ns::^S;",
----------------
I find this case is hard, and not easy for human to follow&verify -- we have 3
points in the target code, and their RefTypes are from the macro arguments.
================
Comment at: clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp:279
+ EXPECT_IMPLICIT_REFERENCES("struct S { ^S(); };", "S ^t;");
+ EXPECT_EXPLICIT_REFERENCES("struct S { ^S(int); };", "S ^t(42);");
+ EXPECT_IMPLICIT_REFERENCES("struct S { ^S(int); };", "S t = ^42;");
----------------
When reading this code, the implicit vs explicit in these 4 consecutive
statements is less obvious than before (each of them has the same length, same
indentation and is a single-line)
================
Comment at: clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp:289
// Unresolved calls marks all the overloads.
- testWalk("void $ambiguous^foo(int); void $ambiguous^foo(char);",
- "template <typename T> void bar() { ^foo(T{}); }");
+ EXPECT_REFERENCES_WITH_TYPES(
+ "void ^foo(int); void ^foo(char);",
----------------
It feels more natural to use `EXPECT_AMBIGUOUS_REFERENCES(TargetCode,
ReferenceCode, Ambiguous)` for this case where all refs are the same type.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D137252/new/
https://reviews.llvm.org/D137252
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
