[PATCH] D111548: [Clang] Add the `annotate_type` attribute

[Martin Böhme via Phabricator via cfe-commits]

mboehme marked an inline comment as done.
mboehme added inline comments.


================
Comment at: clang/include/clang/Basic/AttrDocs.td:6501
+
+The attribute does not have any effect on the semantics of C++ code, neither
+type checking rules, nor runtime semantics. In particular:
----------------
aaron.ballman wrote:
> (Since this is for C as well as C++)
Good point -- done.


================
Comment at: clang/lib/Parse/ParseDecl.cpp:3187-3193
+          // We reject AT_LifetimeBound and AT_AnyX86NoCfCheck, even though they
+          // are type attributes, because we historically haven't allowed these
+          // to be used as type attributes in C++11 / C2x syntax.
+          if (PA.isTypeAttr() && PA.getKind() != ParsedAttr::AT_LifetimeBound &&
+              PA.getKind() != ParsedAttr::AT_AnyX86NoCfCheck)
+            continue;
+          Diag(PA.getLoc(), diag::err_attribute_not_type_attr) << PA;
----------------
aaron.ballman wrote:
> Should this be done as part of D126061 instead?
Thanks for raising this question!

In this patch, I had originally allowed all type attributes to be placed on the decl-specifier-seq because a) I needed this to be true for `annotate_type` so that I could test the attribute in this position, and b) there didn't appear to be any harm in allowing any type attribute instead of just `annotate_type`.

However, subsequent discussion on https://reviews.llvm.org/D126061 has made it clear that not all type attributes should actually be allowed on the decl-specifier-seq -- at least not initially, since they won't currently work correctly in that context (see extensive discussion on that other patch). So I've made the code here more restrictive to allow only `annotate_type` on the decl-specifier-seq (which is all that I need here).

https://reviews.llvm.org/D126061 now contains the change that allows type attributes on the decl-specifier-seq more generally, which is fine because it also contains additional code to disable this behavior again for the specific attributes that need it.

Given that I'm planning to land this patch and https://reviews.llvm.org/D126061 at the same time, it's not _super_ critical where we make this change, but a) the way it's done now makes the patches more logically cohesive, and b) there's always a chance that a patch may need to be reverted, and having the right changes together in one patch will be important then.

Thanks again for bringing this to my attention!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111548/new/

https://reviews.llvm.org/D111548