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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
