ymandel added inline comments.
================
Comment at: clang/unittests/Tooling/SourceCodeTest.cpp:249-258
Visitor.runOverAnnotated(R"cpp(
- #define ATTR __attribute__((deprecated("message")))
- $r[[ATTR
+ $r[[__attribute__((deprecated("message")))
int x;]])cpp");
// Includes attributes and comments together.
Visitor.runOverAnnotated(R"cpp(
+ $r[[__attribute__((deprecated("message")))
----------------
Thanks for trying to fix these! The changed test cases seem to be doing two
things at once: macros and attributes, and I don't remember why. We should test
the behavior separately. So, I think you're new test cases are good
regardless. But, we still want the old tests to test the behavior for
attributes that are hidden behind a macro, since this is used not infrequently
in my exprience.
IIUC, the current code isn't properly handling attributes that appear inside
macros, which is why this fix breaks the code. Specifically, it is not
considering the case that the location in `Attr->getLocation()` is in a macro,
while `Range.getBegin()` is in the original file, and hence the locations are
not comparable. I'm surprised this ever worked, tbh.
My preference would be to update the code, if you know how, so that both the
old and new tests pass. But, I realize this may be a lot to ask. So, at the
least, please comment out, but keep, the old tests; or, copy the old tests to a
new test and mark it disabled. Either way, prefix with a FIXME indicating the
issue.
WDYT?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D137979/new/
https://reviews.llvm.org/D137979
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
