Ping
On Wed, Dec 11, 2013 at 12:34 PM, 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. >> >> FWIW, using the following patch (which is what I was proposing >> originally), 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
