[PATCH] D41553: Support parsing double square-bracket attributes in ObjC

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: include/clang/Basic/Attr.td:239
+  bit IncludeC = includeC;
+}
 
----------------
aaron.ballman wrote:
> rjmccall wrote:
> > aaron.ballman wrote:
> > > rjmccall wrote:
> > > > aaron.ballman wrote:
> > > > > rjmccall wrote:
> > > > > > I have no objection to allowing ObjC attributes to be spelled in [[clang::objc_whatever]] style.  We can debate giving them a more appropriate standard name in the objc namespace at a later time — or even the primary namespace, if we really want to flex our Somewhat Major Language muscles.
> > > > > > 
> > > > > > I feel like this IncludeC is not the best name for this tablegen property.  Perhaps AllowInC?
> > > > > > 
> > > > > > Also, a random positional 1 argument is pretty obscure TableGen design.  Do we really expect to be making very many attributes that intentionally disallow C2x?  What would these be, just C++-specific attributes without C analogues?  It doesn't seem important to prohibit those at the parsing level rather than at the semantic level, since, after all, we are still allowing the GNU-style spelling, and these would still qualified with ``clang::``.
> > > > > > 
> > > > > > I would suggest standardizing in the opposite direction:  instead of forcing attributes to opt in to being allowed in C, we should make attributes that we really don't want to allow in the C2x [[clang::]] namespace be explicit about it.  If there are a lot of C++-specific attributes like that, we can make a ClangCXXOnly subclass.
> > > > > > I have no objection to allowing ObjC attributes to be spelled in [[clang::objc_whatever]] style. We can debate giving them a more appropriate standard name in the objc namespace at a later time — or even the primary namespace, if we really want to flex our Somewhat Major Language muscles.
> > > > > 
> > > > > Thanks -- are you okay with where the attributes are allowed in the syntax? I tried to follow the position they're allowed in C and C++ as closely as I could, but having confirmation would be nice.
> > > > > 
> > > > > As for putting the attributes in the primary namespace, that would be reasonable for using the attributes in ObjC or ObjC++ but not so much for using the same attributes in a pure C or C++ compilation.
> > > > > 
> > > > > > I feel like this IncludeC is not the best name for this tablegen property. Perhaps AllowInC?
> > > > > 
> > > > > I'm fine with that name. I'll change it in D41317 when I commit that.
> > > > > 
> > > > > > Also, a random positional 1 argument is pretty obscure TableGen design. Do we really expect to be making very many attributes that intentionally disallow C2x? What would these be, just C++-specific attributes without C analogues? 
> > > > > 
> > > > > I agree -- I was toying with using a named entity rather than a numeric literal, but I wanted to see how the design shook out in practice once I had a better feel for how many attributes are impacted. I'm glad you recommend going the opposite direction as that was my ultimate goal. :-) Basically, my initial approach is to disallow everything in C and then start enabling the attributes that make sense. At some point, I believe we'll have more attributes in both language modes than not, and then I plan to reverse the meaning of the flag so that an attribute has to opt-out rather than opt-in. I don't expect we'll have many attributes that are disallowed in C2x. I think they'll fall into two categories: C++ attributes that don't make sense in C and attributes that are governed by some other body.
> > > > > 
> > > > > > It doesn't seem important to prohibit those at the parsing level rather than at the semantic level, since, after all, we are still allowing the GNU-style spelling, and these would still qualified with `clang::`.
> > > > > 
> > > > > I think it's important for C targets when `-fdouble-square-bracket-attributes` is not enabled, as the attributes cannot syntactically appear in those positions.
> > > > > 
> > > > > > I would suggest standardizing in the opposite direction
> > > > > 
> > > > > I suspect I will ultimately get there. I chose this approach to be more conservative about what we expose.
> > > > > Thanks -- are you okay with where the attributes are allowed in the syntax? I tried to follow the position they're allowed in C and C++ as closely as I could, but having confirmation would be nice.
> > > > 
> > > > I'll leave comments on the various places.  For the most part, no, I think these are the wrong places; where we allow attributes in ObjC is actually pretty carefully thought-out already, and it's better to follow the places where we parse GNU attributes than to try to imitate the C grammar.
> > > > 
> > > > > As for putting the attributes in the primary namespace, that would be reasonable for using the attributes in ObjC or ObjC++ but not so much for using the same attributes in a pure C or C++ compilation.
> > > > 
> > > > Yes, please ignore that.  I was just idly thinking about what we would do if we were adopting this feature as a more standard thing, i.e. leaving the implementation namespace.  I think we'd want to rename some of the attributes for consistency, and then I think we'd just put them in the objc:: namespace.  None of this affects what we're doing now.
> > > > 
> > > > > I think it's important for C targets when -fdouble-square-bracket-attributes is not enabled, as the attributes cannot syntactically appear in those positions.
> > > > 
> > > > Okay, that's fair.
> > > > 
> > > > > I suspect I will ultimately get there. I chose this approach to be more conservative about what we expose.
> > > > 
> > > > Sure, if this is really just for staging, I won't worry about accidentally leaving attributes out.
> > > > I'll leave comments on the various places. For the most part, no, I think these are the wrong places; where we allow attributes in ObjC is actually pretty carefully thought-out already, and it's better to follow the places where we parse GNU attributes than to try to imitate the C grammar.
> > > 
> > > The C grammar was also carefully thought-out to consistently map the location of the attribute syntax in source code to the entity the attribute appertains to. The basic rule is: the attribute always appertains to what's immediately to the left unless the attribute is at the start of the line, in which case it applies to each declaration in the decl group. This logic was taken from C++ attributes where it's proven very valuable over the past seven or so years. If we follow the GNU syntactic placement, that means this rule of thumb doesn't apply in Objective-C and it negates one of the most powerful aspects of the language feature: syntactic consistency (which is something GNU-style attributes absolutely lacks, to everyone's great annoyance).
> > > 
> > > Can you explain the rationale about why the GNU-style placement is a better approach to the C- and C++-style placement? I think that the syntactic location of the attribute syntax should remain consistent between ObjC constructs and C constructs because both constructs are likely to be mixed together in the same code base. I think we hurt the usability of the language feature by being inconsistent in this fashion (which was one of the reasons we standardized attributes in the first place).
> > The ObjC grammar is overall quite different from the C grammar.  There are no decl groups.  Tags like `@interface` are always declaration introducers, rather than part of the type grammar.  Types are not incidentally declared/defined in the type-specifier of a possible object or function declaration.  More subjectively, nobody looks at the primary Objective-C declarations like `@interface` or `@protocol` and thinks they look like anything else from C or C++, which means consistency is not really an issue.
> > 
> > C++ puts class attributes after the `class-key` because there's literally no other place to put attributes that wouldn't be conflated with an attribute on the object if an object were being simultaneously declared.  The C grammar paints them into a corner.  We don't have that specific problem in ObjC because, as mentioned, ObjC declarations are standalone, not wedged into the object/function declaration grammar.
> > 
> > C++ seems to be inconsistent about where it allows keywords when it does have a proper declaration-introducer.  For example, the grammar for a `namespace` declaration is `'namespace' attribute-specifier-seq? identifier?`, presumably following the lead of `class`.  But the grammar for an alias declaration is `'using' identifier attribute-specifier-seq? '=' defining-type-id`, not `'using' attribute-specifier-seq? identifier '=' defining-type-id`. And of course there are the context-sensitive class attributes (e.g. `final`) that are written in a special position, which is allowable under the grammar because they only need to apply to definitions.
> > 
> > I understand that the GNU/Microsoft/Borland/whatever attribute positions often seem //ad hoc// and that the C++ committee tried to improve on that when standardizing attributes.  In contrast, I think the ObjC community is satisfied with where attributes are written in ObjC declarations today.  When I said that those positions were carefully thought-out, I mean that we've already talked a lot about where attributes should go, and the current positions are the result of those conversations.  That process seems to have been a success; the only complaints I can remember ever getting about attribute parsing have been related to where C requires them, not ObjC.  In general, ObjC pushes attributes to the beginning or (in the case of methods) the end of the declaration, which people see as a good thing: attributes can get quite large, so piling them up on the fringes of the declaration ensures that they don't interfere with reading the more immediately important parts of the declaration.  While C/C++ did not have a choice about where to allow attributes on `class` declarations, objectively it is very awkward to put attributes immediately after the tag because it separates the two most important things in the declaration: the declaration introducer and the name being declared.  That is not something we want to imitate.
> > 
> > I am not just speaking for myself here.  This is where we would like the attributes to appear.
> Thank you for the helpful explanation, I appreciate it.
> 
> Do you have issue with where C is placing these attributes for constructs that are supported by C, or are those also fine for use in ObjC? Basically, I'm wondering whether you would want this syntax at all in ObjC, or whether you're okay with the C placement for C constructs and the ObjC placement for ObjC constructs?
> 
> I'm a bit concerned about this scenario and would like your thoughts:
> ```
> @interface I
> -(void)Foo;
> @end
> 
> @implementation I
> -(void)Foo [[clang::minsize]] { // OK, appertains to the function declaration
> 
> }
> 
> void bar(void) [[clang::minsize]] { // Error, minsize does not appertain to the function type
> 
> }
> @end
> ```
We should just use the C rules for C constructs in ObjC mode, even if they're intermingled with ObjC constructs.  I see your point about the perceived inconsistency, but I don't see a good resolution besides acceptance.  Allowing new locations for C attributes would be a mistake.


https://reviews.llvm.org/D41553