[PATCH] D24886: Add [[clang::suppress(rule, ...)]] attribute

[Aaron Ballman via cfe-commits]

aaron.ballman requested changes to this revision.
aaron.ballman added a comment.
This revision now requires changes to proceed.

I'm generally in favor of this patch, but it's missing some pieces, like test cases. Also, I suspect we will want this attribute to also be written on types, but there are no changes to SemaType.cpp to handle this. Is that something you are planning to support as well?


================
Comment at: include/clang/Basic/Attr.td:1457
@@ +1456,3 @@
+  let Args = [VariadicStringArgument<"Rules">];
+  let Documentation = [Undocumented];
+}
----------------
No new undocumented attributes, please.

================
Comment at: lib/Sema/SemaDeclAttr.cpp:3851
@@ +3850,3 @@
+
+    Rules.push_back(RuleName);
+  }
----------------
Should we diagnose if asked to suppress a diagnostic that we don't support? e.g., someone does `[[clang::suppress("this does not exist")]]`

On the one hand, we want to be forwards-compatible, but on the other hand, I can easily see typos in long string literals. So perhaps we may want to have a diagnostic based on some heuristic. e.g., we diagnose if the spelling is slightly off and we have viable options which we can use typo correction to issue a fixit, or if it's way off base, we silently eat it under the assumption it's a forwards compatibility thing?

================
Comment at: lib/Sema/SemaStmtAttr.cpp:72
@@ +71,3 @@
+                    A.getAttributeSpellingListIndex());
+  return ::new (S.Context) auto(Attr);
+}
----------------
Please construct this directly instead of relying on a copy constructor. Also, same comments about typos apply here as above.


https://reviews.llvm.org/D24886