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

[Ted Kremenek]

Thanks Nuno!  Looks great.  Applied:

http://llvm.org/viewvc/llvm-project?rev=48011&view=rev

On Mar 7, 2008, at 10:23 AM, Nuno Lopes wrote:

> Hi,
>
> So I've tried to fullfill all comments. Additionally I also provide  
> a test for most corner-cases.
> The C++ part is still not handled, but it should be much easier now.
>
> Nuno
>
>
> ----- Original Message ----- From: "Ted Kremenek" <kremenek at apple.com>
> To: "Nuno Lopes" <nunoplopes at sapo.pt>
> Cc: "cfe-dev" <cfe-dev at cs.uiuc.edu>
> Sent: Monday, March 03, 2008 5:47 PM
> Subject: Re: [cfe-dev] handle __attribute__((deprecated))
>
>
>>
>> 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
> <clang_format_attr.txt>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20080307/c5fbd598/attachment.html>