[PATCH] D30009: Add support for '#pragma clang attribute'

Aaron Ballman via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Apr 4 16:31:25 PDT 2017


aaron.ballman added a comment.

Thank you for the update! I think this is getting close, though I have a few more comments inline.



================
Comment at: docs/LanguageExtensions.rst:2338
+The attributes can also be written using the C++11 style syntax, as long
+as only one attribute is specified in the square brackets:
+
----------------
Do we want to have the same restriction for GNU and declspec attributes, that only a single one can appear? If so, we should mention it more generally.


================
Comment at: docs/LanguageExtensions.rst:2420
+- ``variable``: Can be used to apply attributes to variables, including
+  local variables, parameters, and global variables.
+
----------------
Are member variables included in this list? Perhaps this should also mention that it does not apply to Objective-C ivars.


================
Comment at: include/clang/Basic/AttrSubjectMatchRules.h:17
+namespace clang {
+
+namespace attr {
----------------
Can remove the newline.


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:765
+  "'#pragma clang attribute push' regions ends here">;
+def warn_pragm_attribute_no_pop_eof : Warning<"unterminated "
+  "'#pragma clang attribute push' at end of file">,
----------------
Typo: pragm

Is there value in this being in its own warning group from pragma-attribute-unused? It might make sense to disable any pragma attribute warning under the same group name, but I don't feel super strongly about it.


================
Comment at: include/clang/Sema/Sema.h:8160
+  /// '\#pragma clang attribute push' directives to the given declaration.
+  void AddPragmaAttributes(Scope *S, Decl *D);
+
----------------
arphaman wrote:
> aaron.ballman wrote:
> > I worry about new calls to ProcessDeclAttributes() that don't have a nearby call to AddPragmaAttributes(). I wonder if there's a way that we can combat that likely misuse?
> Perhaps we could just always call `AddPragmaAttributes` from `ProcessDeclAttributes` (after the declarator attributes are applied). That would mean that `#pragma clang attribute` will apply its attributes earlier than other pragmas or implicit attributes.
I was wondering whether that would be a reasonable approach. I *think* it seems reasonable, however.


================
Comment at: include/clang/Sema/Sema.h:8160
+  /// '\#pragma clang attribute push' directives to the given declaration.
+  void AddPragmaAttributes(Scope *S, Decl *D);
+
----------------
aaron.ballman wrote:
> arphaman wrote:
> > aaron.ballman wrote:
> > > I worry about new calls to ProcessDeclAttributes() that don't have a nearby call to AddPragmaAttributes(). I wonder if there's a way that we can combat that likely misuse?
> > Perhaps we could just always call `AddPragmaAttributes` from `ProcessDeclAttributes` (after the declarator attributes are applied). That would mean that `#pragma clang attribute` will apply its attributes earlier than other pragmas or implicit attributes.
> I was wondering whether that would be a reasonable approach. I *think* it seems reasonable, however.
Phab won't let me edit my comment for some reason, so replying a second time. I don't think it's a problem for the attributes to be added earlier because attributes are essentially an unordered list on the declarations anyways. If this really is a problem, I would hope that there's a test that will suddenly start failing for us.


================
Comment at: lib/Sema/SemaAttr.cpp:631
+        ProcessDeclAttributeList(S, D, Entry.Attribute);
+        HasFailed = Trap.hasErrorOccurred();
+      }
----------------
arphaman wrote:
> aaron.ballman wrote:
> > Do we only care about errors, or should we also care about warnings? Incorrect attributes that do not affect code generation often are treated as warnings (and the attribute is simply ignored) rather than errors, so the user may want to know about those issues.
> I think it's important to try and get the warnings right as well. I think it would make sense to emit the warnings for each declaration that receives the attribute (including the first one). One issue that I found was that warnings didn't tell the user which declaration was receiving the attribute. The updated patch fixes that by ensuring that warnings are emitted for all receivers and that each warning has an additional note that points to the declaration that receives the attribute.
> 
> This made me also rethink the SFINAE trap completely. I decided to avoid it. Originally I thought that it would be nice to have it so that we can avoid duplicate attribute-related errors when applying an attribute to each declaration.  There were a couple of issues with that approach:
> - Attributes have receiver dependent errors/warnings that should be reported for each declaration. Previously the patch was inconsistent for such errors; it stopped applying the attribute only when the first declaration had a receiver dependent error, but not the others.
> - `ProcessDeclAttributeList` actually applies the attribute, so we can't call it twice. In particular, some attributes might modify the attribute list when reporting warnings during the first call, so the user won't see the warnings when they're reported during the second call.
> 
> The updated patch now always report errors with a note that points to the receiver declaration. As a downside it would mean that when there are actual, non-receiver dependent attribute errors then the user is gonna get a bunch of duplicate errors when applying to multiple declarations.
Thank you, I think this approach makes sense. The downside is unfortunate, but doesn't seem to be actively harmful if I'm understanding properly.


================
Comment at: lib/Sema/SemaAttr.cpp:578
+    return;
+  Diag(PragmaAttributeStack.back().Loc, diag::warn_pragm_attribute_no_pop_eof);
+}
----------------
Perhaps adding a FixIt here would be a kindness?


Repository:
  rL LLVM

https://reviews.llvm.org/D30009





More information about the cfe-commits mailing list