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 <[email protected]> wrote: > > On Dec 11, 2013, at 9:34 AM, Aaron Ballman <[email protected]> wrote: > >> Ping >> >> On Mon, Dec 2, 2013 at 8:50 PM, Aaron Ballman <[email protected]> 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 <[email protected]> wrote: >>>> On Mon, Dec 2, 2013 at 2:35 PM, Aaron Ballman <[email protected]> >>>> wrote: >>>>> >>>>> On Mon, Dec 2, 2013 at 5:25 PM, Richard Smith <[email protected]> >>>>> wrote: >>>>>> On Mon, Dec 2, 2013 at 1:28 PM, Aaron Ballman <[email protected]> >>>>>> 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). > _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
