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

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added inline comments.


================
Comment at: docs/LanguageExtensions.rst:2430
+
+- ``function(is_method)``: Can be used to apply attributes to C++ methods,
+  including operators and constructors/destructors.
----------------
aaron.ballman wrote:
> Instead of `is_method`, I think we should use `is_instance`, `is_member`, or something else. C++ doesn't really use the terminology "method", but Objective-C does, which is why this could be confusing. It might also be good to mention the behavior of a static member functions.
> 
> Whatever terminology gets picked, you should use that phrasing wherever you say "C++ method".
I think `function(is_member)` should work well.


================
Comment at: docs/LanguageExtensions.rst:2443
+
+- ``record(unless(is_union))``: Can be used to apply attributes only to
+  ``struct`` and ``class`` declarations.
----------------
aaron.ballman wrote:
> Does unless() work in other combinations? e.g., can I use record(unless(is_enum))? I'm not saying that it has to, but it would be good to document one way or the other.
No, right now `unless` is very rigid and only works with a select set of match sub rules which are needed for attributes. I have noted that in the docs.


================
Comment at: docs/LanguageExtensions.rst:2448
+
+- ``enum_case``: Can be used to apply attributes to enumerators.
+
----------------
aaron.ballman wrote:
> I'd prefer these to be named `enumeration` and `enumerator`, `enum` and `enum_constant`, or something. `enum_case` is too novel of a term for me.
I think `enum_constant` would work. I was thinking about `enumeration` and `enumerator` before, but they just look too alike so it might be easy to confuse them.


================
Comment at: docs/LanguageExtensions.rst:2486
+
+- ``block``: Can be used to apply attributes to block declarations. This does
+  not include variables/fields of block pointer type.
----------------
aaron.ballman wrote:
> Perhaps name this one `objc_block` to be consistent with the other objective-c features?
Blocks can be used outside of Objective-C.


================
Comment at: docs/LanguageExtensions.rst:2497
+:doc:`individual documentation for that attribute <AttributeReference>`.
+
+The attributes are applied to a declaration only when there would
----------------
aaron.ballman wrote:
> We should also support __declspec attributes shouldn't we (I see no reason to support 2 out of the 3 attribute styles)? If so, the docs should state that, and if not, I'd like to understand why and the docs should still state that.
Sure. A proper consistent style with `__attribute__` as you mentioned earlier is better for us as well due to legacy macros. Now `__attribute__` is required and I also added support for the `__declspec` attributes.


================
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:
> 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.


================
Comment at: lib/Parse/ParsePragma.cpp:1158
+      Diag(Loc, diag::err_pragma_attribute_multiple_attributes);
+      Attrs.getList()->setNext(nullptr);
+    }
----------------
aaron.ballman wrote:
> Is this going to cause a memory leak for those attributes that were parsed but dropped?
Possibly, I fixed this up.


================
Comment at: lib/Sema/SemaAttr.cpp:602-603
+  PragmaAttributeStack.pop_back();
+  // FIXME: Should we check the attribute even when it's not applied to any
+  // declaration?
+}
----------------
aaron.ballman wrote:
> I think that makes sense (I can imagine someone writing the pragma push and pop before writing all their declarations), but I might be misunderstanding the question.
That would make sense, I agree. I added a warning that warns about unapplied attributes in push pop regions. However, my original question was more about correctness of the attribute which we can’t check semantically unless we apply it first. I don’t think it’s really needed though, so I removed the “fixme”.


================
Comment at: lib/Sema/SemaAttr.cpp:631
+        ProcessDeclAttributeList(S, D, Entry.Attribute);
+        HasFailed = Trap.hasErrorOccurred();
+      }
----------------
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.


================
Comment at: test/lit.cfg:287
 
+config.substitutions.append( ('%src_include_dir', config.clang_src_dir + '/include') )
+
----------------
aaron.ballman wrote:
> Why is this change (and the other lit change) required?
`test/Misc/pragma-attribute-supported-attributes-list.test` Uses this path to find Clang's `Attr.td` file.


Repository:
  rL LLVM

https://reviews.llvm.org/D30009