<html><head><base href="x-msg://4/"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; ">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).<div><br><div><div>On Apr 13, 2011, at 11:39 PM, Michael Han wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><span class="Apple-style-span" style="border-collapse: separate; font-family: Helvetica; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-border-horizontal-spacing: 0px; -webkit-border-vertical-spacing: 0px; -webkit-text-decorations-in-effect: none; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; font-size: medium; "><div lang="EN-US" link="blue" vlink="purple" style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div class="WordSection1" style="page: WordSection1; "><div style="margin-top: 0in; margin-right: 0in; margin-bottom: 0.0001pt; margin-left: 0in; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); ">Hi Ted,<o:p></o:p></span></div><div style="margin-top: 0in; margin-right: 0in; margin-bottom: 0.0001pt; margin-left: 0in; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); "><o:p> </o:p></span></div><div style="margin-top: 0in; margin-right: 0in; margin-bottom: 0.0001pt; margin-left: 0in; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); ">Thanks for the comments. I have attached the revised patch which adds<span class="Apple-converted-space"> </span></span><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); ">a<span class="Apple-converted-space"> </span></span><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); ">hasParameterOrArguments to AttributeList and use that in Sema checking.</span><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); "><o:p></o:p></span></div><div style="margin-top: 0in; margin-right: 0in; margin-bottom: 0.0001pt; margin-left: 0in; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); "><o:p> </o:p></span></div><div><div style="margin-top: 0in; margin-right: 0in; margin-bottom: 0.0001pt; margin-left: 0in; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); ">Cheers<o:p></o:p></span></div><div style="margin-top: 0in; margin-right: 0in; margin-bottom: 0.0001pt; margin-left: 0in; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); ">~Michael<o:p></o:p></span></div></div><div style="margin-top: 0in; margin-right: 0in; margin-bottom: 0.0001pt; margin-left: 0in; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125); "><o:p> </o:p></span></div><div><div style="border-right-style: none; border-bottom-style: none; border-left-style: none; border-width: initial; border-color: initial; border-top-style: solid; border-top-color: rgb(181, 196, 223); border-top-width: 1pt; padding-top: 3pt; padding-right: 0in; padding-bottom: 0in; padding-left: 0in; "><div style="margin-top: 0in; margin-right: 0in; margin-bottom: 0.0001pt; margin-left: 0in; font-size: 12pt; font-family: 'Times New Roman', serif; "><b><span style="font-size: 10pt; font-family: Tahoma, sans-serif; ">From:</span></b><span style="font-size: 10pt; font-family: Tahoma, sans-serif; "><span class="Apple-converted-space"> </span>Ted Kremenek [mailto:kremenek@apple.com]<span class="Apple-converted-space"> </span><br><b>Sent:</b><span class="Apple-converted-space"> </span>Thursday, April 14, 2011 12:03 PM<br><b>To:</b><span class="Apple-converted-space"> </span>Michael Han<br><b>Cc:</b><span class="Apple-converted-space"> </span><a href="mailto:cfe-commits@cs.uiuc.edu" style="color: blue; text-decoration: underline; ">cfe-commits@cs.uiuc.edu</a><br><b>Subject:</b><span class="Apple-converted-space"> </span>Re: [cfe-commits] [Patch] Improve diagnostics on gnu attributes<o:p></o:p></span></div></div></div><div style="margin-top: 0in; margin-right: 0in; margin-bottom: 0.0001pt; margin-left: 0in; font-size: 12pt; font-family: 'Times New Roman', serif; "><o:p> </o:p></div><div style="margin-top: 0in; margin-right: 0in; margin-bottom: 0.0001pt; margin-left: 0in; font-size: 12pt; font-family: 'Times New Roman', serif; ">Hi Michael,<o:p></o:p></div><div><div style="margin-top: 0in; margin-right: 0in; margin-bottom: 0.0001pt; margin-left: 0in; font-size: 12pt; font-family: 'Times New Roman', serif; "><o:p> </o:p></div></div><div><div style="margin-top: 0in; margin-right: 0in; margin-bottom: 0.0001pt; margin-left: 0in; font-size: 12pt; font-family: 'Times New Roman', serif; ">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.<o:p></o:p></div></div><div><div style="margin-top: 0in; margin-right: 0in; margin-bottom: 0.0001pt; margin-left: 0in; font-size: 12pt; font-family: 'Times New Roman', serif; "><o:p> </o:p></div></div><div><div style="margin-top: 0in; margin-right: 0in; margin-bottom: 0.0001pt; margin-left: 0in; font-size: 12pt; font-family: 'Times New Roman', serif; ">Instead of doing:<o:p></o:p></div></div><div><div style="margin-top: 0in; margin-right: 0in; margin-bottom: 0.0001pt; margin-left: 0in; font-size: 12pt; font-family: 'Times New Roman', serif; "><o:p> </o:p></div></div><div><div><div style="margin-top: 0in; margin-right: 0in; margin-bottom: 0.0001pt; margin-left: 0in; font-size: 12pt; font-family: 'Times New Roman', serif; ">+ if (Attr.getParameterName() || Attr.getNumArgs() != 0) {<o:p></o:p></div></div></div><div><div style="margin-top: 0in; margin-right: 0in; margin-bottom: 0.0001pt; margin-left: 0in; font-size: 12pt; font-family: 'Times New Roman', serif; "><o:p> </o:p></div></div><div><div style="margin-top: 0in; margin-right: 0in; margin-bottom: 0.0001pt; margin-left: 0in; font-size: 12pt; font-family: 'Times New Roman', serif; ">how about something like:<o:p></o:p></div></div><div><div style="margin-top: 0in; margin-right: 0in; margin-bottom: 0.0001pt; margin-left: 0in; font-size: 12pt; font-family: 'Times New Roman', serif; "><o:p> </o:p></div></div><div><div style="margin-top: 0in; margin-right: 0in; margin-bottom: 0.0001pt; margin-left: 0in; font-size: 12pt; font-family: 'Times New Roman', serif; "> if (Attr.hasParameterOrArguments())<o:p></o:p></div></div><div><div style="margin-top: 0in; margin-right: 0in; margin-bottom: 0.0001pt; margin-left: 0in; font-size: 12pt; font-family: 'Times New Roman', serif; "><o:p> </o:p></div></div><div><div style="margin-top: 0in; margin-right: 0in; margin-bottom: 0.0001pt; margin-left: 0in; font-size: 12pt; font-family: 'Times New Roman', serif; ">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. <o:p></o:p></div></div><div><div style="margin-top: 0in; margin-right: 0in; margin-bottom: 0.0001pt; margin-left: 0in; font-size: 12pt; font-family: 'Times New Roman', serif; "><o:p> </o:p></div></div><div><div style="margin-top: 0in; margin-right: 0in; margin-bottom: 0.0001pt; margin-left: 0in; font-size: 12pt; font-family: 'Times New Roman', serif; ">Ted<o:p></o:p></div></div><div><div style="margin-top: 0in; margin-right: 0in; margin-bottom: 0.0001pt; margin-left: 0in; font-size: 12pt; font-family: 'Times New Roman', serif; "><o:p> </o:p></div></div><div><div><div><div style="margin-top: 0in; margin-right: 0in; margin-bottom: 0.0001pt; margin-left: 0in; font-size: 12pt; font-family: 'Times New Roman', serif; ">On Apr 10, 2011, at 9:17 PM, Michael Han wrote:<o:p></o:p></div></div><div style="margin-top: 0in; margin-right: 0in; margin-bottom: 0.0001pt; margin-left: 0in; font-size: 12pt; font-family: 'Times New Roman', serif; "><br><br><o:p></o:p></div><div><div><div style="margin-top: 0in; margin-right: 0in; margin-bottom: 0.0001pt; margin-left: 0in; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; ">Consider this code:<o:p></o:p></span></div></div><div><div style="margin-top: 0in; margin-right: 0in; margin-bottom: 0.0001pt; margin-left: 0in; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; "> <o:p></o:p></span></div></div><div><div style="margin-top: 0in; margin-right: 0in; margin-bottom: 0.0001pt; margin-left: 0in; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; ">int a;<o:p></o:p></span></div></div><div><div style="margin-top: 0in; margin-right: 0in; margin-bottom: 0.0001pt; margin-left: 0in; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; ">__attribute__((noreturn(a))) void foo();<o:p></o:p></span></div></div><div><div style="margin-top: 0in; margin-right: 0in; margin-bottom: 0.0001pt; margin-left: 0in; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; "> <o:p></o:p></span></div></div><div><div style="margin-top: 0in; margin-right: 0in; margin-bottom: 0.0001pt; margin-left: 0in; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; ">Clang does not emit any warning or errors on this code; gcc would report wrong number of arguments for the attribute specified.<o:p></o:p></span></div></div><div><div style="margin-top: 0in; margin-right: 0in; margin-bottom: 0.0001pt; margin-left: 0in; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; ">The attached patch fixed this by emit a “takes no argument” error for clang.<o:p></o:p></span></div></div><div><div style="margin-top: 0in; margin-right: 0in; margin-bottom: 0.0001pt; margin-left: 0in; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; "> <o:p></o:p></span></div></div><div><div style="margin-top: 0in; margin-right: 0in; margin-bottom: 0.0001pt; margin-left: 0in; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; ">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<o:p></o:p></span></div></div><div><div style="margin-top: 0in; margin-right: 0in; margin-bottom: 0.0001pt; margin-left: 0in; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; "> <o:p></o:p></span></div></div><div><div style="margin-top: 0in; margin-right: 0in; margin-bottom: 0.0001pt; margin-left: 0in; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; ">Cheers<o:p></o:p></span></div></div><div><div style="margin-top: 0in; margin-right: 0in; margin-bottom: 0.0001pt; margin-left: 0in; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; ">~Michael<o:p></o:p></span></div></div><div><div style="margin-top: 0in; margin-right: 0in; margin-bottom: 0.0001pt; margin-left: 0in; font-size: 12pt; font-family: 'Times New Roman', serif; "><span style="font-size: 11pt; font-family: Calibri, sans-serif; "> <o:p></o:p></span></div></div><div style="margin-top: 0in; margin-right: 0in; margin-bottom: 0.0001pt; margin-left: 0in; font-size: 12pt; font-family: 'Times New Roman', serif; "><attributes.fix.patch>_______________________________________________<br>cfe-commits mailing list<br><a href="mailto:cfe-commits@cs.uiuc.edu" style="color: blue; text-decoration: underline; ">cfe-commits@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" style="color: blue; text-decoration: underline; ">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><o:p></o:p></div></div></div><div style="margin-top: 0in; margin-right: 0in; margin-bottom: 0.0001pt; margin-left: 0in; font-size: 12pt; font-family: 'Times New Roman', serif; "><o:p> </o:p></div></div></div><span><attr.arg.fix.revised.patch></span></div></span></blockquote></div><br></div></body></html>