tdl-g requested changes to this revision.
tdl-g added a comment.
This revision now requires changes to proceed.
Just one comment about the tests.
================
Comment at: clang/unittests/Tooling/RangeSelectorTest.cpp:196
+static void testAfterMacroArg(StringRef Code) {
+ const std::string Ref = "ref";
----------------
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.
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
