Looks good. One nit: please add a doxygen comment for this new method, and space it from the other method declarations (just like in the rest of the class).
On Apr 13, 2011, at 11:39 PM, Michael Han wrote: > Hi Ted, > > Thanks for the comments. I have attached the revised patch which adds a > hasParameterOrArguments to AttributeList and use that in Sema checking. > > Cheers > ~Michael > > From: Ted Kremenek [mailto:[email protected]] > Sent: Thursday, April 14, 2011 12:03 PM > To: Michael Han > Cc: [email protected] > Subject: Re: [cfe-commits] [Patch] Improve diagnostics on gnu attributes > > Hi Michael, > > From looking at ParseDecl.cpp, ParseGNUAttributes, I think the difference > between the parameter and the arguments is that the parameter is an > identifier and the arguments are expressions. Essentially, the parameter is > the first argument that is guaranteed to be an identifier. > > Instead of doing: > > + if (Attr.getParameterName() || Attr.getNumArgs() != 0) { > > how about something like: > > if (Attr.hasParameterOrArguments()) > > which would just an inline method in Attribute that expands to the logic you > wrote. Right now you're basically copy-pasting a "complex" predicate in > multiple places, and that is the perfect use of an inline method that > summarizes your intent and makes the client code simpler. > > Ted > > On Apr 10, 2011, at 9:17 PM, Michael Han wrote: > > > Consider this code: > > int a; > __attribute__((noreturn(a))) void foo(); > > Clang does not emit any warning or errors on this code; gcc would report > wrong number of arguments for the attribute specified. > The attached patch fixed this by emit a “takes no argument” error for clang. > > Alternatively parser can be updated so the first identifier immediately > following the attribute name could be treated as argument, instead of > “parameter” (what is the difference between them?). This would require a lot > more work to update the parser and the AttributeList so I am not sure if that > is the right thing to do for now. Please review the patch thanks > > Cheers > ~Michael > > <attributes.fix.patch>_______________________________________________ > cfe-commits mailing list > [email protected] > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits > > <attr.arg.fix.revised.patch>
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
