<div dir="ltr">On Mon, Dec 2, 2013 at 1:28 PM, Aaron Ballman <span dir="ltr"><<a href="mailto:aaron@aaronballman.com" target="_blank">aaron@aaronballman.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">The nothrow and const attributes have a curious construct that does<br>

not appear with any other attributes. Both of them check whether the<br>
attribute has already been applied, and if it has, it silently fails<br>
to reapply the attribute.  Eg)<br>
<br>
  if (NoThrowAttr *Existing = D->getAttr<NoThrowAttr>()) {<br>
    if (Existing->getLocation().isInvalid())<br>
      Existing->setRange(Attr.getRange());<br>
  } else {<br>
    D->addAttr(::new (S.Context)<br>
               NoThrowAttr(Attr.getRange(), S.Context,<br>
                           Attr.getAttributeSpellingListIndex()));<br>
  }<br>
<br>
While this makes some sense, two things in particular are worrisome:<br>
<br>
1) We lose syntactic information because we are losing the as-written<br>
attribute information. This happens relatively frequently, but any<br>
time we can cut that down would be good.<br>
2) This is not consistently applied across attributes.<br>
<br>
I am wondering whether it is reasonable to remove the check for the<br>
existing attribute from the nothrow and const attributes, and simply<br>
apply multiple times. Since neither of these attributes accept<br>
arguments, it seems like this would be a harmless action. However, I<br>
may not have the full picture; this is the original commit message:<br>
<br>
dgregor  6/15/2011 1:45:11 AM<br>
Don't add redundant FormatAttr, ConstAttr, or NoThrowAttr attributes,<br>
either imlicitly (for builtins) or explicitly (due to multiple<br>
specification of the same attributes). Fixes <rdar://problem/9612060>.<br>
<br>
(Note, I am not suggesting any modifications for FormatAttr.)<br>
<br>
Thanks!</blockquote><div><br></div><div>Here's the testcase from that change:</div><div><br></div><div><div>+int printf(const char * restrict, ...) __attribute__((__format__ (__printf__, 1, 2)));</div><div>+</div><div>
+void rdar9612060(void) {</div><div>+  printf("%s", 2); // expected-warning{{conversion specifies type 'char *' but the argument has type 'int'}}</div><div>+}</div></div><div><br></div><div>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.</div>
</div></div></div>