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

[Aaron Ballman]

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