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


================
Comment at: lib/Parse/ParseObjc.cpp:231
 
+  MaybeParseCXX11Attributes(attrs);
   MaybeSkipAttributes(tok::objc_interface);
----------------
Okay, so this is allowing attributes after `@interface` but before the class name, e.g. `@interface [[attr]] X`.  I don't think we actually want this; we really want attributes on classes to be written prior to the `@keyword`, and I don't think that's spelling-specific in any way.  Frankly, I see this is an awkward C-ism that the ObjC grammar just doesn't really require.


================
Comment at: lib/Parse/ParseObjc.cpp:726
+      ParsedAttributesWithRange Attrs(AttrFactory);
+      MaybeParseCXX11Attributes(Attrs);
+
----------------
So this is specifically allowing `@property [[attr]] (copy) id x;`, whereas today we require you to write `@property (copy) [[attr]] id x;` or something like it.  Again, I don't think we actually want to allow attributes in this position.



================
Comment at: lib/Parse/ParseObjc.cpp:1388
+  // Parse square bracket attributes after the method name.
+  MaybeParseCXX11Attributes(methodAttrs);
+
----------------
So, this is not actually reliably after the method name, it's just after the first selector chunk but before its colon.  This is definitely not an acceptable place to write attributes.  They should be allowed here if this is a nullary selector, i.e. there is no colon; the right place to do that is just a few lines below this, where you can see we parse GNU attributes and then finish the parse.


================
Comment at: lib/Parse/ParseObjc.cpp:1434
       MaybeParseGNUAttributes(paramAttrs);
-      ArgInfo.ArgAttrs = paramAttrs.getList();
     }
 
----------------
ObjC parameter syntax is really its own weird thing.  I think this is the right place to allow attributes, after the parenthesized type but before the parameter-variable name (which is optional).  And of course we also allow them within the parentheses, but hopefully that just falls out from parsing the type.


================
Comment at: lib/Parse/ParseObjc.cpp:1456
+    MaybeParseCXX11Attributes(paramAttrs);
+    ArgInfo.ArgAttrs = paramAttrs.getList();
+
----------------
So, this is wrong because it's potentially conflated with the end of the method, which we parse below when the tokens run out.  You'll see that we allow GNU-style attributes there, and we should allow `[[attrs]]` there as well.


================
Comment at: lib/Parse/ParseObjc.cpp:2041
 
+  MaybeParseCXX11Attributes(attrs);
   MaybeSkipAttributes(tok::objc_protocol);
----------------
Like with `@interface`, I think we don't want to allow attributes here.


https://reviews.llvm.org/D41553