nothrow and const Attribtues

[Aaron Ballman]

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?

~Aaron