compnerd 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")))
----------------
ymandel wrote:
> 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?
Unfortunately, I couldn't figure out how to track through the expansion so I am
going to take you up on the commented out case option.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D137979/new/
https://reviews.llvm.org/D137979
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
