aaron.ballman added a comment.

In https://reviews.llvm.org/D23895#529275, @thakis wrote:

> > There are things missing from this patch as well. To wit:
>
> > 
>
> > - Attributes.h needs to know about this attribute syntax, and 
> > `hasAttribute()` needs to support it (this hooks into `__has_attribute` 
> > support).
>
> > - AttributeList.h should be updated to expose the parsed attribute syntax.
>
> > - Both of these changes should be threaded through ClangAttrEmitter.cpp.
>
>
> All these are already part of this patch as far as I can tell. What am I 
> missing?


This is what happens when I split my review into two sessions. I think I was 
just tired, because I now see that you did change Attributes.h and 
AttributeList.h properly. Sorry about the noise! However, I think 
`GenerateHasAttrSpellingStringSwitch()` should be updated (the Microsoft 
variety shouldn't work if Microsoft extensions are disabled), but otherwise 
ClangAttrEmitter.cpp seems reasonable.


================
Comment at: lib/Parse/ParseDecl.cpp:1431
@@ -1427,4 +1430,3 @@
 // class-key affects the type instead of the variable.
-void Parser::handleDeclspecAlignBeforeClassKey(ParsedAttributesWithRange 
&Attrs,
-                                               DeclSpec &DS,
-                                               Sema::TagUseKind TUK) {
+// Also, Microsoft-style [attributes] affect the type instead of the variable.
+// This function moves attributes that should apply to the type off DS to 
Attrs.
----------------
thakis wrote:
> aaron.ballman wrote:
> > Is this documented somewhere, or just what you gather from experimentation?
> Experimentation.
It might be good to clarify that in the comment so we don't come along later 
and think it's a hard-and-fast rule.

================
Comment at: utils/TableGen/ClangAttrEmitter.cpp:2372
@@ -2363,3 +2371,3 @@
                 .Case("Declspec", 2)
-                .Case("Keyword", 3)
-                .Case("Pragma", 4)
+                .Case("Microsoft", 3)
+                .Case("Keyword", 4)
----------------
thakis wrote:
> aaron.ballman wrote:
> > I think (but my memory is a bit fuzzy here) that these values need to match 
> > the ones from AttributeList.h. I think it's a bug that Pragma was at 4 
> > rather than 5, but since __has_attribute doesn't care about pragmas (or 
> > context sensitive keywords), it's likely benign.
> I can change pragma to map to one more in a separate patch if you want.
Yeah, or I can take care of it, either way is fine by me.


https://reviews.llvm.org/D23895



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to