<html><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><br><div><div>On Mar 1, 2008, at 12:28 AM, Chris Lattner wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><span class="Apple-style-span" style="border-collapse: separate; color: rgb(0, 0, 0); font-family: Helvetica; font-size: 13px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: auto; 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: 0; ">Should this emit a diagnostic if it's not a function? Ted, can you please review the rest of the 'HandleFormatAttribute' method?</span></blockquote></div><br><div>Hi Nuno,</div><div><br class="webkit-block-placeholder"></div><div>I've reviewed the version of HandleFormatAttribute that Chris committed to the clang tree from your latest patch. Thanks again for submitting this patch. It's a great contribution. I have a few comments:</div><div><br class="webkit-block-placeholder"></div><div><br class="webkit-block-placeholder"></div><div><div><font class="Apple-style-span" face="Courier" size="3"><span class="Apple-style-span" style="font-size: 11px;"> // FIXME: in C++ the implicit 'this' function parameter also counts.</span></font></div> <div><font class="Apple-style-span" face="Courier" size="3"><span class="Apple-style-span" style="font-size: 11px;"> // the index must start in 1 and the limit is numargs+1</span></font></div> <div><font class="Apple-style-span" face="Courier" size="3"><span class="Apple-style-span" style="font-size: 11px;"> unsigned NumArgs = Fn->getNumParams()<b>+1</b>; // +1 for ...</span></font></div><div><br class="webkit-block-placeholder"></div><div><br class="webkit-block-placeholder"></div><div>This looks incorrect for all code except non-static C++ methods, of which we do not currently handle. The code after this frequently checks against NumArgs, which will be incorrect (off-by-one). Also, I'm not certain how we are actually going to handle declarations for class members, but chances are we are not going to use FunctionDecls. I would just remove the increment for now, and leave the FIXME. When I first looked at this code I was also a little confused; I thought you were trying to offset for a deficiency in the AST, rather than the fact that this is a game that the format attribute plays in GCC (i.e., offsetting for the implicit "this" pointer):</div><div><br class="webkit-block-placeholder"></div><div> "<span class="Apple-style-span" style="font-family: Times; font-size: 16px; ">Since non-static C++ methods have an implicit <code>this </code>argument, the arguments of such methods should be counted from two, not one..."</span></div><div><font class="Apple-style-span" face="Times" size="4"><span class="Apple-style-span" style="font-size: 16px;"><br class="webkit-block-placeholder"></span></font></div><div>Make a comment that this is is mirroring GCC behavior when it comes to C++ non-static methods and the format attribute. It also seems that offsets to the base index would have to be handled as well. I would just punt on this for now until we have Decls for C++ methods in place. I'm a little concerned about doing this right for C++ non-static methods; having little fudges like these to indices and ranges is a great source for off-by-one and buffer overflow errors if not all of the code obeys the rule. This behavior just seems like a GCC kludge, with the ASTs being tied to closely to the implementation of how methods are code generated (thus forcing the attribute specification itself to be broken); in reality, the "this" pointer would never be used as a format string, so we probably should just pretend that it isn't there when processing the format argument indices. This means that in the code below your original increment of NumArgs that we should instead *decrement* the parsed indices when handling C++ non-static methods. That way, the FormatAttr object always stores the canonicalized indices for the *explicit* function arguments (again, ignoring the "this" argument).</div><div><br class="webkit-block-placeholder"></div><div>More comments:</div></div><div><br class="webkit-block-placeholder"></div><div><div><font class="Apple-style-span" face="Courier" size="3"><span class="Apple-style-span" style="font-size: 11px;"> Expr *IdxExpr = static_cast<Expr *>(rawAttr->getArg(0));</span></font></div> <div><font class="Apple-style-span" face="Courier" size="3"><span class="Apple-style-span" style="font-size: 11px;"> <b>llvm::APSInt Idx(32);</b></span></font></div> <div><font class="Apple-style-span" face="Courier" size="3"><span class="Apple-style-span" style="font-size: 11px;"> if (!IdxExpr->isIntegerConstantExpr(Idx, Context)) {</span></font></div> <div><font class="Apple-style-span" face="Courier" size="3"><span class="Apple-style-span" style="font-size: 11px;"> Diag(rawAttr->getLoc(), diag::err_attribute_argument_n_not_int,</span></font></div> <div><font class="Apple-style-span" face="Courier" size="3"><span class="Apple-style-span" style="font-size: 11px;"> "format", std::string("2"), IdxExpr->getSourceRange());</span></font></div> <div><font class="Apple-style-span" face="Courier" size="3"><span class="Apple-style-span" style="font-size: 11px;"> return;</span></font></div> <div><font class="Apple-style-span" face="Courier" size="3"><span class="Apple-style-span" style="font-size: 11px;"> }</span></font></div><div><br class="webkit-block-placeholder"></div><div><br class="webkit-block-placeholder"></div><div>Please do not hardcode "32" or the signedness of Idx, both which could be target specific. Please query these using Context::getTypeSize(IdxExpr) and IdxExpr->getType()-isUnsignedIntegerType().</div><div><br class="webkit-block-placeholder"></div><div>For the following block, please add a big comment indicating what it is trying to do:</div><div><br></div><div><div><br></div> <div><font class="Apple-style-span" face="Courier" size="3"><span class="Apple-style-span" style="font-size: 11px;"> Expr *IdxExpr = static_cast<Expr *>(rawAttr->getArg(0));</span></font></div> <div><font class="Apple-style-span" face="Courier" size="3"><span class="Apple-style-span" style="font-size: 11px;"> llvm::APSInt Idx(32);</span></font></div> <div><font class="Apple-style-span" face="Courier" size="3"><span class="Apple-style-span" style="font-size: 11px;"> if (!IdxExpr->isIntegerConstantExpr(Idx, Context)) {</span></font></div> <div><font class="Apple-style-span" face="Courier" size="3"><span class="Apple-style-span" style="font-size: 11px;"> Diag(rawAttr->getLoc(), diag::err_attribute_argument_n_not_int,</span></font></div> <div><font class="Apple-style-span" face="Courier" size="3"><span class="Apple-style-span" style="font-size: 11px;"> "format", std::string("2"), IdxExpr->getSourceRange());</span></font></div> <div><font class="Apple-style-span" face="Courier" size="3"><span class="Apple-style-span" style="font-size: 11px;"> return;</span></font></div> <div><font class="Apple-style-span" face="Courier" size="3"><span class="Apple-style-span" style="font-size: 11px;"> }</span></font></div> <div><font class="Apple-style-span" face="Courier" size="3"><span class="Apple-style-span" style="font-size: 11px;"><br></span></font></div> <div><font class="Apple-style-span" face="Courier" size="3"><span class="Apple-style-span" style="font-size: 11px;"> if (Idx.getZExtValue() < 1 || Idx.getZExtValue() > NumArgs) {</span></font></div> <div><font class="Apple-style-span" face="Courier" size="3"><span class="Apple-style-span" style="font-size: 11px;"> Diag(rawAttr->getLoc(), diag::err_attribute_argument_out_of_bounds,</span></font></div> <div><font class="Apple-style-span" face="Courier" size="3"><span class="Apple-style-span" style="font-size: 11px;"> "format", std::string("2"), IdxExpr->getSourceRange());</span></font></div> <div><font class="Apple-style-span" face="Courier" size="3"><span class="Apple-style-span" style="font-size: 11px;"> return;</span></font></div> <div><font class="Apple-style-span" face="Courier" size="3"><span class="Apple-style-span" style="font-size: 11px;"> }</span></font></div> <div><font class="Apple-style-span" face="Courier" size="3"><span class="Apple-style-span" style="font-size: 11px;"><br></span></font></div> <div><font class="Apple-style-span" face="Courier" size="3"><span class="Apple-style-span" style="font-size: 11px;"> Expr *FirstArgExpr = static_cast<Expr *>(rawAttr->getArg(1));</span></font></div> <div><font class="Apple-style-span" face="Courier" size="3"><span class="Apple-style-span" style="font-size: 11px;"> llvm::APSInt FirstArg(32);</span></font></div> <div><font class="Apple-style-span" face="Courier" size="3"><span class="Apple-style-span" style="font-size: 11px;"> if (!FirstArgExpr->isIntegerConstantExpr(FirstArg, Context)) {</span></font></div> <div><font class="Apple-style-span" face="Courier" size="3"><span class="Apple-style-span" style="font-size: 11px;"> Diag(rawAttr->getLoc(), diag::err_attribute_argument_n_not_int,</span></font></div> <div><font class="Apple-style-span" face="Courier" size="3"><span class="Apple-style-span" style="font-size: 11px;"> "format", std::string("3"), FirstArgExpr->getSourceRange());</span></font></div> <div><font class="Apple-style-span" face="Courier" size="3"><span class="Apple-style-span" style="font-size: 11px;"> return;</span></font></div> <div><font class="Apple-style-span" face="Courier" size="3"><span class="Apple-style-span" style="font-size: 11px;"> }</span></font></div> <div><font class="Apple-style-span" face="Courier" size="3"><span class="Apple-style-span" style="font-size: 11px;"><br></span></font></div> <div><font class="Apple-style-span" face="Courier" size="3"><span class="Apple-style-span" style="font-size: 11px;"> if (FormatLen == 8 && !memcmp(Format, "strftime", 8)) {</span></font></div> <div><font class="Apple-style-span" face="Courier" size="3"><span class="Apple-style-span" style="font-size: 11px;"> if (FirstArg.getZExtValue() != 0) {</span></font></div> <div><font class="Apple-style-span" face="Courier" size="3"><span class="Apple-style-span" style="font-size: 11px;"> Diag(rawAttr->getLoc(), diag::err_format_strftime_third_parameter,</span></font></div> <div><font class="Apple-style-span" face="Courier" size="3"><span class="Apple-style-span" style="font-size: 11px;"> FirstArgExpr->getSourceRange());</span></font></div> <div><font class="Apple-style-span" face="Courier" size="3"><span class="Apple-style-span" style="font-size: 11px;"> return;</span></font></div> <div><font class="Apple-style-span" face="Courier" size="3"><span class="Apple-style-span" style="font-size: 11px;"> }</span></font></div> <div><font class="Apple-style-span" face="Courier" size="3"><span class="Apple-style-span" style="font-size: 11px;"> } else if (FirstArg.getZExtValue() > NumArgs) {</span></font></div> <div><font class="Apple-style-span" face="Courier" size="3"><span class="Apple-style-span" style="font-size: 11px;"> Diag(rawAttr->getLoc(), diag::err_attribute_argument_out_of_bounds,</span></font></div> <div><font class="Apple-style-span" face="Courier" size="3"><span class="Apple-style-span" style="font-size: 11px;"> "format", std::string("3"), FirstArgExpr->getSourceRange());</span></font></div> <div><font class="Apple-style-span" face="Courier" size="3"><span class="Apple-style-span" style="font-size: 11px;"> return;</span></font></div> <div><font class="Apple-style-span" face="Courier" size="3"><span class="Apple-style-span" style="font-size: 11px;"> }</span></font></div><div><br class="webkit-block-placeholder"></div><div><br class="webkit-block-placeholder"></div></div><div>As far as comments are concerned, it would be great to see a citation link to the appropriate references. For example, the following link provides GCC's documentation on format argument attributes:</div><div><br class="webkit-block-placeholder"></div><div> <a href="http://gcc.gnu.org/onlinedocs/gcc-4.2.3/gcc/Function-Attributes.html#index-g_t_0040code_007bformat_007d-function-attribute-1880">http://gcc.gnu.org/onlinedocs/gcc-4.2.3/gcc/Function-Attributes.html#index-g_t_0040code_007bformat_007d-function-attribute-1880</a></div><div><br class="webkit-block-placeholder"></div><div>Adding this link in a comment would be helpful so that people know what this code is trying to implement. This mirrors documentation through the rest of clang where we try to cite either the C standard or the appropriate reference. Also, please add comments above checks like "<span class="Apple-style-span" style="font-family: Courier; font-size: 11px; ">if (FirstArg.getZExtValue() != 0)<span class="Apple-style-span" style="font-family: Helvetica; font-size: 13px; ">" where the argument is not suppose to be zero and *why*.</span></span></div><div><br class="webkit-block-placeholder"></div><div>There is no harm in adding comments that document behavior; for example, before seeing this code I did not know that the format attribute supported the __strftime__ and __strfmon__ archetypes. I don't think GCC always supported these archetypes, so adding some comments saying what functionality this adds to the format attribute would be incredibly useful to other people browsing this code (which is one of the goals of clang).</div><div><br class="webkit-block-placeholder"></div><div>Overall, this patch looks very good, and like your previous patches, it's a great contribution to clang.</div><div><br></div><div>Ted</div></div></body></html>