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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet marked an inline comment as done.
kadircet added a comment.

In D137252#3907665 <https://reviews.llvm.org/D137252#3907665>, @hokein wrote:

> 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);

I see your point but this is not really a regression, right? that's the way the tests were written in the previous version as well. i am not sure how we can make this more explicit without introducing a bunch of boilerplate into each test case.

> 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;

That is one of the points I disliked the most as well, maybe we should keep the old test syntax for these cases (with abbreviations), i.e. rather than putting the types at the end, we'll put them into the annotations. WDYT?

> 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);

These macros are not for single references, but rather for single reference types. e.g. if all the references are of same kind.



================
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",
----------------
hokein wrote:
> a bug in existing code? by looking at this example, we should use `EXPECT_AMBIGUOUS_REFERENCES(Code, Code, RefType::Ambiguous, RefType::Ambiguous, RefType::Ambiguous)`
no, that's actually a change in behaviour (in the base patch) but wasn't reflected here.


================
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;",
----------------
hokein wrote:
> 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.
i agree. just to make my proposal in the main discussion more concrete, i am suggesting this:
```
EXPECT_REFERENCES_WITH_TYPES(
      R"cpp(
    namespace ns {
      void $A^x(); void $E^x(int); void $A^x(char);
    })cpp",
      "// c++-header\n using ns::^x; void foo() { x(3); }");
```


================
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;");
----------------
hokein wrote:
> 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)
i am not sure if there's much difference between having them in the code vs in the macro names here actually.

the case on the left hand side of the diff is exactly the same, you've got implicit vs explicit right next to points, each with same length. IMO the author/reviewer needs to pay ~the same level of attention in either case.


================
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);",
----------------
hokein wrote:
> It feels more natural to use  `EXPECT_AMBIGUOUS_REFERENCES(TargetCode, ReferenceCode,  Ambiguous)` for this case where all refs are the same type.
right, that one should've been just `EXPECT_AMBIGUOUS_REFERENCES`, a residue left behind while i was tweaking the code


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137252/new/

https://reviews.llvm.org/D137252