On Sun, Jun 17, 2012 at 6:07 PM, Richard Smith <[email protected]> wrote: > Mostly just nitpicking: > >> Index: include/clang/Basic/DiagnosticParseKinds.td >> =================================================================== >> --- include/clang/Basic/DiagnosticParseKinds.td (revision 158272) >> +++ include/clang/Basic/DiagnosticParseKinds.td (working copy) >> @@ -481,6 +481,9 @@ >> "introducing an attribute">; >> def err_alignas_pack_exp_unsupported : Error< >> "pack expansions in alignment specifiers are not supported yet">; >> +def err_unknown_ms_declspec : Error<"unrecognized __declspec attribute %0">; > > This should probably be a Warning, and InGroup<UnknownAttributes>. It > would also seems sensible to mirror the wording of > warn_unknown_attribute_ignored: "unknown __declspec attribute %0 > ignored".
The reason I made it an error was because it's an error in VS as well. Since it pertains to __declspec, which is already MS-specific, I figured the behavior should match. Given that reasoning, do you still think we should prefer a warning over an error? >> Index: include/clang/Sema/AttributeList.h >> =================================================================== >> --- include/clang/Sema/AttributeList.h (revision 158272) >> +++ include/clang/Sema/AttributeList.h (working copy) >> @@ -80,6 +80,9 @@ >> /// availability attribute. >> unsigned IsAvailability : 1; >> >> + /// True if this attribute is actually a type attribute >> + unsigned IsTypeAttribute : 1; > > Sean Hunt has a patch for this class which will replace the > DeclspecAttribute and CXX0XAttribute flags with an enum for the > syntactic form used for the attribute. It would make sense to add this > as a value in the enum, instead of adding another flag (so you're > going to need to wait a short while for Sean's patch to land), since > these attributes don't use the syntactic form of __declspec > attributes. The comment (and possibly the name) could also be improved > to indicate that these are Microsoft type attribute keywords (and give > a couple of examples in the comment). Fair enough. > A general comment about this function: the MS documentation suggests > that multiple attributes may be specified in the same __declspec, for > instance "__declspec(dllexport naked)". It appears that the old code > supported this and the new code doesn't. Is that intentional? Great catch (it was unintentional) -- looks like I need another testcase for this, too. >> Index: lib/Sema/SemaDeclAttr.cpp >> =================================================================== >> --- lib/Sema/SemaDeclAttr.cpp (revision 158272) >> +++ lib/Sema/SemaDeclAttr.cpp (working copy) >> @@ -4162,8 +4164,11 @@ >> if (Attr.isInvalid()) >> return; >> >> - if (Attr.isDeclspecAttribute() && !isKnownDeclSpecAttr(Attr)) >> - // FIXME: Try to deal with other __declspec attributes! >> + // Type attributes are still treated as declaration attributes by >> + // ParseMicrosoftTypeAttributes and ParseBorlandTypeAttributes. We don't >> + // want to process them, however, because we will simply warn about >> ignoring >> + // them. So instead, we will bail out early. >> + if (Attr.isTypeAttribute()) >> return; > > Can you check Attr.isUsedAsTypeAttr() instead, in the code path which > produces the warning? This would presumably allow the checks for > AT_address_space, AT_opencl_image_access etc in > ProcessInheritableDeclAttr to be removed too. Yes, I bet I can -- I'll look into it for the next patch. Thanks again! ~Aaron _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
