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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
