[PATCH] D137979: parse: process GNU and standard attributes on top-level decls

Tue Nov 15 09:59:18 PST 2022

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