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
