[PATCH] D55628: Add support for "labels" on push/pop directives in #pragma clang attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/docs/LanguageExtensions.rst:2667
+Because multiple push directives can be nested, if you're writing a macro that
+expands to ``_Pragma("clang attribute")`` it's good hygiene to add a label to
+your push/pop directives. A pop directive with a label will pop the innermost
----------------
The documentation reads like the label is optional, but the examples make it look required (because all examples have the label present). Can you clarify what happens when the label is omitted and keep an example that shows it?


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:840-842
+def err_pragma_attribute_stack_mismatch_label : Error<
+  "'#pragma clang attribute pop %0' with no matching "
+  "'#pragma clang attribute push %0'">;
----------------
Can this be combined with the diagnostic above using `%select` (given that the wording is basically identical)?


================
Comment at: clang/include/clang/Sema/Sema.h:8504
+  void ActOnPragmaAttributeEmptyPush(SourceLocation PragmaLoc,
+                                     IdentifierInfo *PushLabel);
 
----------------
Can you sprinkle some const-correctness around to make this `const IdentifierInfo *`?


================
Comment at: clang/lib/Sema/SemaAttr.cpp:648
 
-  for (const PragmaAttributeEntry &Entry :
-       PragmaAttributeStack.back().Entries) {
-    if (!Entry.IsUsed) {
-      assert(Entry.Attribute && "Expected an attribute");
-      Diag(Entry.Attribute->getLoc(), diag::warn_pragma_attribute_unused)
-          << Entry.Attribute->getName();
-      Diag(PragmaLoc, diag::note_pragma_attribute_region_ends_here);
+  // Dig back through the stack trying to find the group that corrisponds to
+  // PushLabel. Note that this works fine if no label is present, think of
----------------
corrisponds -> corresponds


================
Comment at: clang/lib/Sema/SemaAttr.cpp:659
+          Diag(Entry.Attribute->getLoc(), diag::warn_pragma_attribute_unused)
+              << Entry.Attribute->getName();
+          Diag(PragmaLoc, diag::note_pragma_attribute_region_ends_here);
----------------
You can pass in `*Entry.Attribute` here instead of calling `getName()` and it will properly quote the results.


================
Comment at: clang/test/Sema/pragma-attribute-label.c:7
+
+#pragma clang attribute pop // expected-error{{'#pragma clang attribute pop' with no matching '#pragma clang attribute push'}}
+#pragma clang attribute pop NOT_MY_LABEL // expected-error{{'#pragma clang attribute pop NOT_MY_LABEL' with no matching '#pragma clang attribute push NOT_MY_LABEL'}}
----------------
Should we really treat this as an error? It seems to me that this should be a warning because pop without a label could be viewed as "I don't care what I'm popping, just pop it". Still worth warning about, but maybe not worth stopping a build over.


================
Comment at: clang/test/Sema/pragma-attribute-label.c:15
+// Out of order!
+#pragma clang attribute pop MY_LABEL
+
----------------
I feel like this should be diagnosed, perhaps even as an error. The user provided labels but then got the push and pop order wrong when explicitly saying what to pop. That seems more likely to be a logic error on the user's part.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55628