[cfe-dev] handle __attribute__((deprecated))

[Ted Kremenek]

On Mar 1, 2008, at 12:28 AM, Chris Lattner wrote:

> Should this emit a diagnostic if it's not a function?  Ted, can you  
> please review the rest of the 'HandleFormatAttribute' method?

Hi Nuno,

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:


   // FIXME: in C++ the implicit 'this' function parameter also counts.
   // the index must start in 1 and the limit is numargs+1
   unsigned NumArgs = Fn->getNumParams()+1; // +1 for ...


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):

   "Since non-static C++ methods have an implicit this argument, the  
arguments of such methods should be counted from two, not one..."

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).

More comments:

   Expr *IdxExpr = static_cast<Expr *>(rawAttr->getArg(0));
   llvm::APSInt Idx(32);
   if (!IdxExpr->isIntegerConstantExpr(Idx, Context)) {
     Diag(rawAttr->getLoc(), diag::err_attribute_argument_n_not_int,
            "format", std::string("2"), IdxExpr->getSourceRange());
     return;
   }


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().

For the following block, please add a big comment indicating what it  
is trying to do:


   Expr *IdxExpr = static_cast<Expr *>(rawAttr->getArg(0));
   llvm::APSInt Idx(32);
   if (!IdxExpr->isIntegerConstantExpr(Idx, Context)) {
     Diag(rawAttr->getLoc(), diag::err_attribute_argument_n_not_int,
            "format", std::string("2"), IdxExpr->getSourceRange());
     return;
   }

   if (Idx.getZExtValue() < 1 || Idx.getZExtValue() > NumArgs) {
     Diag(rawAttr->getLoc(), diag::err_attribute_argument_out_of_bounds,
            "format", std::string("2"), IdxExpr->getSourceRange());
     return;
   }

   Expr *FirstArgExpr = static_cast<Expr *>(rawAttr->getArg(1));
   llvm::APSInt FirstArg(32);
   if (!FirstArgExpr->isIntegerConstantExpr(FirstArg, Context)) {
     Diag(rawAttr->getLoc(), diag::err_attribute_argument_n_not_int,
            "format", std::string("3"), FirstArgExpr->getSourceRange());
     return;
   }

   if (FormatLen == 8 && !memcmp(Format, "strftime", 8)) {
     if (FirstArg.getZExtValue() != 0) {
       Diag(rawAttr->getLoc(),  
diag::err_format_strftime_third_parameter,
              FirstArgExpr->getSourceRange());
       return;
     }
   } else if (FirstArg.getZExtValue() > NumArgs) {
     Diag(rawAttr->getLoc(), diag::err_attribute_argument_out_of_bounds,
            "format", std::string("3"), FirstArgExpr->getSourceRange());
     return;
   }


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:

    http://gcc.gnu.org/onlinedocs/gcc-4.2.3/gcc/Function-Attributes.html#index-g_t_0040code_007bformat_007d-function-attribute-1880

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 "if (FirstArg.getZExtValue() != 0)" where the argument is  
not suppose to be zero and *why*.

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).

Overall, this patch looks very good, and like your previous patches,  
it's a great contribution to clang.

Ted
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20080303/4159856b/attachment.html>