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

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

In D137252#3907743 <https://reviews.llvm.org/D137252#3907743>, @kadircet wrote:

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

In the previous version, we can easily distinguish the `TargetCode` and `ReferenceCode` by looking at annotation `$name` in the code, while in the current version, we have to memorize the function argument order.

One alternative will be to merge `TargetCode` and `ReferenceCode` together:

  int ^x; // using ^ as a reference point to the target symbol
  
  int y = [[]]x; // using [[]]] as a ref point to the reference.



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

It can work, probably it is ok, it looks like -- we keep the original annotation approach (with shorter names), and provide some syntax sugar for common cases. (though I personally prefer to use the shortened-annotation approach everywhere). We might want a third opinion @sammccall.

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

I see, I got confusing when reading the  `EXPECT_REFERENCES_WITH_TYPES("", "", Ambiguous, Ambiguous)` code.



================
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",
----------------
kadircet wrote:
> 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.
I don't get it. My understanding is 

1. this patch is basically a NFC change on top of the https://reviews.llvm.org/D135859 and in https://reviews.llvm.org/D135859, we use three ambiguous ref types
2. and here, we are changing one of ambiguous ref types to an explicit type.

1 and 2 is conflicted, do you mean in the base version, it should be `ambiguous, explicit, ambiguous`?


================
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;");
----------------
kadircet wrote:
> 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.
I think there is a difference -- in the previous version, you don't need to read the function name, you can see the reference type from the `^` point in the reference code snippet, while in the current version, you first identify the `^` point, and *move* our eyeball to the macro name (extra step).

Maybe we can use shorter names for these macros (e.g. EXPECT_IMPLICIT, EXPECT_EXPLICIT?).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137252