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

[Aaron Ballman]

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