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

[Nuno Lopes]

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 
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: clang_format_attr.txt
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20080307/450d00ed/attachment.txt>