r224256, with the additional test point. Making the same/different decls behave consistently, and the template idea, are on my list; the inconsistency is clearly a Bad Thing so I'll get to it at some point but maybe not right away. Thanks, --paulr
> -----Original Message----- > From: Aaron Ballman [mailto:[email protected]] > Sent: Monday, December 15, 2014 6:39 AM > To: Robinson, Paul > Cc: [email protected] > Subject: Re: [PATCH] Diagnose 'optnone' versus conflicting attrs on > another decl > > On Fri, Dec 12, 2014 at 1:05 PM, Robinson, Paul > <[email protected]> wrote: > > Updated patch to add a new note pointing out the conflicting attribute. > > Left the templatizing and using the new note on dllimport/dllexport to > > some future patch. How does this look? > > I'd like to see one more test like: > > __attribute__((always_inline)) // expected-warning {{}} > __attribute__((minsize)) // expected-warning {{}} > void f(); > > __attribute__((optnone)) // expected-note 2 {{}} > void f() {} > > To make sure it behaves properly in the presence of both minsize and > always_inline. With that, LGTM! Also, I agree with the approach of > downgrading the error case into a warning when present on the same > declaration (or changing the merge case to be an error, so long as > it's consistent). > > ~Aaron > > > Thanks, > > --paulr > > > >> -----Original Message----- > >> From: Aaron Ballman [mailto:[email protected]] > >> Sent: Friday, December 12, 2014 6:56 AM > >> To: Robinson, Paul > >> Cc: [email protected] > >> Subject: Re: [PATCH] Diagnose 'optnone' versus conflicting attrs on > >> another decl > >> > >> On Thu, Dec 11, 2014 at 7:27 PM, Robinson, Paul > >> <[email protected]> wrote: > >> > Diagnose when attribute 'optnone' conflicts with attributes on a > >> > different declaration. > >> > > >> > I modeled this on how dllimport/dllexport are handled; specifically, > >> > I made 'optnone' continue to "win" over 'always_inline' and > 'minsize'. > >> > This preserves the previous "winner" behavior but adds the > diagnostics > >> > that were requested. > >> > > >> > I have two questions about all this. > >> > > >> > First, the diagnostics point to the attribute being ignored, but not > >> > to the attribute that caused it to be ignored. Is that okay? This > >> > would be a problem for dllimport/dllexport as well as the new cases. > >> > >> No reason it can't do both. You have the original attribute to be > >> merged, and you have the existing attribute on the declaration, so > >> you've got everything you need. You can either pass the range in to > >> the call to Diag (like I'm doing below), or you could create a note > >> diagnostic and pass it in there. > >> > >> > Second, now that there are 5 similar cases (dllimport v. dllexport, > >> > plus optnone v. always_inline/minsize) it seems like there could be > >> > an opportunity to refactor some of that diagnostic checking into a > >> > templated helper function, along the lines of mergeVisibilityAttr. > >> > Should I pursue that? (Not clear it's much of a win... but maybe.) > >> > >> It's possible -- you could do something like: > >> > >> template <typename IncompatTy, typename AttrTy> > >> AttrTy *Sema::mergeWithABetterNameHere(Decl *D, SourceRange R, unsigned > >> ASLI) { > >> if (const auto *Old = D->getAttr<IncompatTy>()) { > >> Diag(Old->getLocation(), diag::warn_attribute_ignored) << R << Old; > >> return nullptr; > >> } > >> > >> if (D->hasAttr<AttrTy>()) > >> return nullptr; > >> > >> return ::new (Context) AttrTy(Range, Context, ASLI); > >> } > >> > >> Then you can use this in place of mergeAlwaysInlineAttr and > >> mergeMinSizeAttr. May even be able to template parameter packs to > >> handle the incompat check for mergeOptimizeNoneAttr if you wanted to > >> get fancy. > >> > >> The patch LGTM as-is; I suspect the merge logic will need some > >> attention at some point, but this isn't particularly burdensome. > >> > >> ~Aaron _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
