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