nothrow and const Attribtues

[Richard Smith]

On Mon, Dec 2, 2013 at 2:35 PM, Aaron Ballman <aaron at aaronballman.com>wrote:

> On Mon, Dec 2, 2013 at 5:25 PM, Richard Smith <richard at metafoo.co.uk>
> wrote:
> > On Mon, Dec 2, 2013 at 1:28 PM, Aaron Ballman <aaron at aaronballman.com>
> > wrote:
> >>
> >> The nothrow and const attributes have a curious construct that does
> >> not appear with any other attributes. Both of them check whether the
> >> attribute has already been applied, and if it has, it silently fails
> >> to reapply the attribute.  Eg)
> >>
> >>   if (NoThrowAttr *Existing = D->getAttr<NoThrowAttr>()) {
> >>     if (Existing->getLocation().isInvalid())
> >>       Existing->setRange(Attr.getRange());
> >>   } else {
> >>     D->addAttr(::new (S.Context)
> >>                NoThrowAttr(Attr.getRange(), S.Context,
> >>                            Attr.getAttributeSpellingListIndex()));
> >>   }
> >>
> >> While this makes some sense, two things in particular are worrisome:
> >>
> >> 1) We lose syntactic information because we are losing the as-written
> >> attribute information. This happens relatively frequently, but any
> >> time we can cut that down would be good.
> >> 2) This is not consistently applied across attributes.
> >>
> >> I am wondering whether it is reasonable to remove the check for the
> >> existing attribute from the nothrow and const attributes, and simply
> >> apply multiple times. Since neither of these attributes accept
> >> arguments, it seems like this would be a harmless action. However, I
> >> may not have the full picture; this is the original commit message:
> >>
> >> dgregor  6/15/2011 1:45:11 AM
> >> Don't add redundant FormatAttr, ConstAttr, or NoThrowAttr attributes,
> >> either imlicitly (for builtins) or explicitly (due to multiple
> >> specification of the same attributes). Fixes <rdar://problem/9612060>.
> >>
> >> (Note, I am not suggesting any modifications for FormatAttr.)
> >>
> >> Thanks!
> >
> >
> > Here's the testcase from that change:
> >
> > +int printf(const char * restrict, ...) __attribute__((__format__
> > (__printf__, 1, 2)));
> > +
> > +void rdar9612060(void) {
> > +  printf("%s", 2); // expected-warning{{conversion specifies type 'char
> *'
> > but the argument has type 'int'}}
> > +}
> >
> > It seems the problem that this change was trying to avoid was where an
> > attribute is implicitly added because the function is a builtin, and then
> > explicitly added. I suspect this is redundant when initially building the
> > attribute -- mergeDeclAttributes will not propagate the attribute from
> the
> > implicitly-declared builtin onto the explicitly-declared function.
>
> That makes sense - you would want to avoid duplicate diagnostics in
> that case. I believe you are correct that it is redundant. Quick
> testing by removing the code from const and nothrow handlers and
> testing demonstrates no regressions. Do you have a better way for me
> to test this?


You could look at the output of -ast-dump and check that there are no
duplicate attributes.

I wonder what we should do about a case like this:

  void f(const char*, ...) __attribute__((format(printf, 1, 2)))
__attribute__((format(printf, 1, 2)));
  void g() { f("%s"); }
  void f(const char*, ...);
  void h() { f("%s"); }

I think removing the deduplication will result in g() producing two
warnings, but h() only producing one (because mergeDeclAttributes will only
propagate one of them from the earlier declaration to the later one).
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131202/95983274/attachment.html>