[PATCH] D60872: Add new warning knob for unknown attribute namespaces

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman marked 7 inline comments as done.
aaron.ballman added inline comments.


================
Comment at: lib/Sema/SemaDeclAttr.cpp:8623
+  } else if (A.hasScope()) {
+#define ATTR(A)
+#define ATTR_NAMESPACE(A) .Case(#A, false)
----------------
dblaikie wrote:
> dblaikie wrote:
> > Not sure how it's done elsewhere - but I'd sink these #defines down to immediately before the #include
> I think most other uses of .inc files only #define the macros they need to - assuming the others aren't defined, rather than explicitly providing an empty definition? (but I'm not sure - I guess that means teh .inc file needs a #ifndef X \ #define X #endif - but perhaps .inc files usually have those?)
This file is generated without a default for `ATTR`, so there are a few places where we have to define it to an empty macro. I could add it to the emitter and remove a few of these spurious definitions, but that can be done in a follow-up.


================
Comment at: lib/Sema/SemaDeclAttr.cpp:8623-8624
+  } else if (A.hasScope()) {
+#define ATTR(A)
+#define ATTR_NAMESPACE(A) .Case(#A, false)
+    if (llvm::StringSwitch<bool>(A.getScopeName()->getName())
----------------
aaron.ballman wrote:
> dblaikie wrote:
> > dblaikie wrote:
> > > Not sure how it's done elsewhere - but I'd sink these #defines down to immediately before the #include
> > I think most other uses of .inc files only #define the macros they need to - assuming the others aren't defined, rather than explicitly providing an empty definition? (but I'm not sure - I guess that means teh .inc file needs a #ifndef X \ #define X #endif - but perhaps .inc files usually have those?)
> This file is generated without a default for `ATTR`, so there are a few places where we have to define it to an empty macro. I could add it to the emitter and remove a few of these spurious definitions, but that can be done in a follow-up.
I think it's pretty awkward either way, so I'll go with your approach.


================
Comment at: lib/Sema/SemaDeclAttr.cpp:8629-8630
+      Warning = diag::warn_unknown_attribute_namespace_ignored;
+#undef ATTR_NAMESPACE
+#undef ATTR
+  } else if (A.isC2xAttribute() || A.isCXX11Attribute()) {
----------------
dblaikie wrote:
> Should AttrList.inc handle undefing all its attributes - so all its users don't have to?
Good catch -- it already does that for me. I'll remove.


================
Comment at: utils/TableGen/ClangAttrEmitter.cpp:2553
 
+    void copyAttrNamespaces(std::set<std::string> &S) const {
+      for (const Record *R : Attrs) {
----------------
dblaikie wrote:
> Maybe "addAttrNamespacesTo" (not clear whether this would overwrite the contens of the std::set, or add to it)
Good catch, I missed renaming it after a signature change where it was accepting an iterator instead of the container directly.


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

https://reviews.llvm.org/D60872