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