nothrow and const Attribtues

[Aaron Ballman]

Thank you for the feedback! I'll look into the tablegen idea at some
point, I'm sure.

Applied the patch in r197864.

~Aaron

On Fri, Dec 20, 2013 at 10:36 PM, Douglas Gregor <dgregor at apple.com> wrote:
>
> On Dec 11, 2013, at 9:34 AM, Aaron Ballman <aaron at aaronballman.com> wrote:
>
>> Ping
>>
>> On Mon, Dec 2, 2013 at 8:50 PM, Aaron Ballman <aaron at aaronballman.com> wrote:
>>> I'm not suggesting removing the dedupe for format -- that is handled
>>> by mergeFormatAttr (which is called from handleFormatAttr to actually
>>> add the attribute to the decl).
>>>
>>> But it is an interesting enough question with regards to the other
>>> attributes -- this strikes me as a general attribute problem. Some
>>> attributes are acceptable to have duplicates specified (such as the
>>> thread safety attributes), others are acceptable only if the duplicate
>>> attribute is different (such as the work group attributes), and still
>>> others it is unacceptable to duplicate at all. This makes me wonder if
>>> it's something we could tablegen? Perhaps a bit that is on by default
>>> which allows us to warn if an attribute has already been applied? If
>>> the bit is off, then the semantic handler can do custom checking if
>>> desired, or simply not diagnose it. The downside to this is something
>>> I've run up against already: there's no way to map parsed attributes
>>> to semantic attributes, so there's no easy way to see if an
>>> AttributeList object is already applied to a Decl (esp since there's
>>> not a one-to-one mapping of parsed to semantic attributes). So if this
>>> is an good idea, it's not something that's trivial to implement.
>
> I'm generally in favor of anything that makes more of the attribute system declarative, and I think this is a good candidate. It's reasonable to have the bit to handle the duplication checks for simple attributes (where the attribute argument matters), then have Sema cope with de-duplication for the more interesting attributes.
>
>
>>> FWIW, using the following patch (which is what I was proposing
>>> originally),
>
> This patch looks fine as well; it's an improvement over the status quo.
>
>         - Doug
>
>
>>> it appears as though builtins are still properly merged.
>>> Testcase:
>>>
>>> int abs(int) { return 0; }
>>>
>>> yields:
>>>
>>> TranslationUnitDecl 0x2a2100 <<invalid sloc>>
>>> |-TypedefDecl 0x2a23d0 <<invalid sloc>> __builtin_va_list 'char *'
>>> |-FunctionDecl 0x2a2490 <E:\Aaron Ballman\Desktop\test.c:1:5> abs 'int (int)' ex
>>> tern
>>> | |-ParmVarDecl 0x2a2500 <<invalid sloc>> 'int'
>>> | |-NoThrowAttr 0x2a2540 <col:5>
>>> | `-ConstAttr 0x2a2570 <col:5>
>>> `-FunctionDecl 0x2a2590 prev 0x2a2490 <col:1, col:26> abs 'int (int)'
>>>  |-ParmVarDecl 0x2a2410 <col:9, col:12> 'int'
>>>  |-CompoundStmt 0x2a2688 <col:14, col:26>
>>>  | `-ReturnStmt 0x2a2678 <col:16, col:23>1 error generated.
>>>
>>>  |   `-IntegerLiteral 0x2a2658 <col:23> 'int' 0
>>>  |-NoThrowAttr 0x2a2620 <col:5>
>>>  `-ConstAttr 0x2a2640 <col:5>
>>>
>>> ~Aaron
>>>
>>> On Mon, Dec 2, 2013 at 7:33 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>>>> 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).
>