[PATCH] Downgrade 'optnone' attribute conflicts to warning

[Aaron Ballman]

On Tue, Jan 13, 2015 at 10:21 AM, Robinson, Paul
<Paul_Robinson at playstation.sony.com> wrote:
>
>
>> -----Original Message-----
>> From: cfe-commits-bounces at cs.uiuc.edu [mailto:cfe-commits-
>> bounces at cs.uiuc.edu] On Behalf Of Aaron Ballman
>> Sent: Tuesday, January 13, 2015 6:55 AM
>> To: reviews+D6933+public+e7e8eb108c4fe81a at reviews.llvm.org
>> Cc: llvm cfe
>> Subject: Re: [PATCH] Downgrade 'optnone' attribute conflicts to warning
>>
>> On Mon, Jan 12, 2015 at 3:31 PM, Paul Robinson
>> <Paul_Robinson at playstation.sony.com> wrote:
>> > Hi aaron.ballman,
>> >
>> > When attribute 'optnone' appears on the same declaration with a
>> > conflicting attribute, warn about the conflict and pick a "winning"
>> > attribute to preserve, instead of emitting an error.  This matches the
>> > behavior when the conflicting attributes are on different declarations.
>> >
>> > Along the way I discovered that conflicts involving __forceinline were
>> > reported as 'always_inline' (alternate spellings for the same
>> > attribute) so I hacked in a way to provide the spelling used in the
>> > declaration. There must be a better way, though...
>>
>> LGTM, with one possible suggestion below.
>>
>> >
>> > http://reviews.llvm.org/D6933
>> >
>> > Files:
>> >   include/clang/Sema/Sema.h
>> >   lib/Sema/SemaDecl.cpp
>> >   lib/Sema/SemaDeclAttr.cpp
>> >   test/SemaCXX/attr-optnone.cpp
>> >
>> > EMAIL PREFERENCES
>> >   http://reviews.llvm.org/settings/panel/emailpreferences/
>>
>> > Index: include/clang/Sema/Sema.h
>> > ===================================================================
>> > --- include/clang/Sema/Sema.h
>> > +++ include/clang/Sema/Sema.h
>> > @@ -2009,6 +2009,7 @@
>> >    SectionAttr *mergeSectionAttr(Decl *D, SourceRange Range, StringRef
>> Name,
>> >                                  unsigned AttrSpellingListIndex);
>> >    AlwaysInlineAttr *mergeAlwaysInlineAttr(Decl *D, SourceRange Range,
>> > +                                          StringRef Name,
>> >                                            unsigned
>> AttrSpellingListIndex);
>> >    MinSizeAttr *mergeMinSizeAttr(Decl *D, SourceRange Range,
>> >                                  unsigned AttrSpellingListIndex);
>> > Index: lib/Sema/SemaDecl.cpp
>> > ===================================================================
>> > --- lib/Sema/SemaDecl.cpp
>> > +++ lib/Sema/SemaDecl.cpp
>> > @@ -2155,7 +2155,8 @@
>> >                                         AttrSpellingListIndex,
>> >                                         IA->getSemanticSpelling());
>> >    else if (const auto *AA = dyn_cast<AlwaysInlineAttr>(Attr))
>> > -    NewAttr = S.mergeAlwaysInlineAttr(D, AA->getRange(),
>> AttrSpellingListIndex);
>> > +    NewAttr = S.mergeAlwaysInlineAttr(D, AA->getRange(), AA-
>> >getSpelling(),
>> > +                                      AttrSpellingListIndex);
>> >    else if (const auto *MA = dyn_cast<MinSizeAttr>(Attr))
>> >      NewAttr = S.mergeMinSizeAttr(D, MA->getRange(),
>> AttrSpellingListIndex);
>> >    else if (const auto *OA = dyn_cast<OptimizeNoneAttr>(Attr))
>> > Index: lib/Sema/SemaDeclAttr.cpp
>> > ===================================================================
>> > --- lib/Sema/SemaDeclAttr.cpp
>> > +++ lib/Sema/SemaDeclAttr.cpp
>> > @@ -3141,9 +3141,14 @@
>> >  }
>> >
>> >  AlwaysInlineAttr *Sema::mergeAlwaysInlineAttr(Decl *D, SourceRange
>> Range,
>> > +                                              StringRef Name,
>> >                                                unsigned
>> AttrSpellingListIndex) {
>> >    if (OptimizeNoneAttr *Optnone = D->getAttr<OptimizeNoneAttr>()) {
>> > -    Diag(Range.getBegin(), diag::warn_attribute_ignored) <<
>> "'always_inline'";
>> > +    SmallString<40> QuotedName;
>> > +    QuotedName = "'";
>> > +    QuotedName += Name;
>> > +    QuotedName += "'";
>> > +    Diag(Range.getBegin(), diag::warn_attribute_ignored) << QuotedName;
>> >      Diag(Optnone->getLocation(), diag::note_conflicting_attribute);
>> >      return nullptr;
>> >    }
>>
>> You could pass in the name as an IdentifierInfo * and ditch the manual
>> quoting. I realize that Attr::getSpelling() returns a StringRef, but
>> getting the IdentifierInfo should be low-cost and slightly less ugly.
>
> Yes, with an IdentifierInfo then I just << it into Diag and the quoting
> happens for me, definitely cleaner.
>
> That works when calling mergeAlwaysInlineAttr from handleAlwaysInlineAttr,
> but it's also called from mergeDeclAttribute in SemaDecl.cpp.  In that case
> I have an InheritableAttr, not an AttributeList, and I didn't see a way to
> get an IdentifierInfo out of it.  That's why I went with passing the string.
>
> Is there a way to get IdentifierInfo out of InheritableAttr?

Context.Idents.get(theAttr->getSpelling())

~Aaron