[PATCH] D137252: [include-cleaner] Testing helpers for ast walking

[Haojian Wu via Phabricator via cfe-commits]

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