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? 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
optnone-part3.diff
Description: optnone-part3.diff
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
