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

[Robinson, Paul]

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:aaron.ballman at gmail.com]
> Sent: Monday, December 15, 2014 6:39 AM
> To: Robinson, Paul
> Cc: cfe-commits at cs.uiuc.edu
> Subject: Re: [PATCH] Diagnose 'optnone' versus conflicting attrs on
> another decl
> 
> On Fri, Dec 12, 2014 at 1:05 PM, Robinson, Paul
> <Paul_Robinson at playstation.sony.com> 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: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