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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: include/clang/Basic/Attr.td:239
+  bit IncludeC = includeC;
+}
 
----------------
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).


https://reviews.llvm.org/D41553