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

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

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