ymandel added inline comments.
================
Comment at: clang/unittests/Tooling/RangeSelectorTest.cpp:196
+static void testAfterMacroArg(StringRef Code) {
+ const std::string Ref = "ref";
----------------
tdl-g wrote:
> If this helper function took an "expected" parameter I might consider it
> self-explanatory, but as it is, it's extremely non-obvious what it does
> without reading the body in detail--which means the tests that use it are not
> particularly readable either. (Each test reads like "here's the input data,
> make sure something unspecified is true about it.") Specifically, this
> function searches for a reference to the variable "y" and ensures that
> after() finds the character after it. So at a minimum, document that.
>
> I'm also trying to decide if this helper is too similar to the
> implementation--tests should not just restate what the production code does,
> they should find some other way to validate the behavior. Do you think this
> is sufficiently different from the prod implementation to be meaningful? If
> so, that's fine. If not, maybe the helper should just take an expected byte
> offset, be renamed to "afterYHasByteOffset", and then each test would be a
> bit more readable?
>
> Up to you.
I reduced the helper to just getting the spelling location at the given offset.
the rest of the code I've inlined into the tests. That helper is still
substantially less complicated than what happens in the production code, and I
don't see any better way to find the location, unless we want to do an offset
from, say, the braces of the function. that avoids macro-related calculations,
but also disconnects from the var. So, I think it's just a tradeoff rather than
an improvement.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D89468/new/
https://reviews.llvm.org/D89468
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
