[PATCH] Diagnose 'optnone' versus conflicting attrs on another decl

[Robinson, Paul]

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:aaron.ballman at gmail.com]
> Sent: Friday, December 12, 2014 6:56 AM
> To: Robinson, Paul
> Cc: cfe-commits at cs.uiuc.edu
> Subject: Re: [PATCH] Diagnose 'optnone' versus conflicting attrs on
> another decl
> 
> On Thu, Dec 11, 2014 at 7:27 PM, Robinson, Paul
> <Paul_Robinson at playstation.sony.com> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: optnone-part3.diff
Type: application/octet-stream
Size: 8470 bytes
Desc: optnone-part3.diff
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20141212/59d022ad/attachment.obj>