[PATCH] Downgrade 'optnone' attribute conflicts to warning
Robinson, Paul
Paul_Robinson at playstation.sony.com
Tue Jan 13 07:21:17 PST 2015
> -----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?
Thanks,
--paulr
>
>
> > @@ -3191,34 +3196,23 @@
> >
> > static void handleAlwaysInlineAttr(Sema &S, Decl *D,
> > const AttributeList &Attr) {
> > - if (checkAttrMutualExclusion<OptimizeNoneAttr>(S, D, Attr))
> > - return;
> > -
> > - D->addAttr(::new (S.Context)
> > - AlwaysInlineAttr(Attr.getRange(), S.Context,
> > - Attr.getAttributeSpellingListIndex()));
> > + if (AlwaysInlineAttr *Inline = S.mergeAlwaysInlineAttr(
> > + D, Attr.getRange(), Attr.getName()->getName(),
> > + Attr.getAttributeSpellingListIndex()))
> > + D->addAttr(Inline);
> > }
> >
> > -static void handleMinSizeAttr(Sema &S, Decl *D,
> > - const AttributeList &Attr) {
> > - if (checkAttrMutualExclusion<OptimizeNoneAttr>(S, D, Attr))
> > - return;
> > -
> > - D->addAttr(::new (S.Context)
> > - MinSizeAttr(Attr.getRange(), S.Context,
> > - Attr.getAttributeSpellingListIndex()));
> > +static void handleMinSizeAttr(Sema &S, Decl *D, const AttributeList
> &Attr) {
> > + if (MinSizeAttr *MinSize = S.mergeMinSizeAttr(
> > + D, Attr.getRange(), Attr.getAttributeSpellingListIndex()))
> > + D->addAttr(MinSize);
> > }
> >
> > static void handleOptimizeNoneAttr(Sema &S, Decl *D,
> > const AttributeList &Attr) {
> > - if (checkAttrMutualExclusion<AlwaysInlineAttr>(S, D, Attr))
> > - return;
> > - if (checkAttrMutualExclusion<MinSizeAttr>(S, D, Attr))
> > - return;
> > -
> > - D->addAttr(::new (S.Context)
> > - OptimizeNoneAttr(Attr.getRange(), S.Context,
> > - Attr.getAttributeSpellingListIndex()));
> > + if (OptimizeNoneAttr *Optnone = S.mergeOptimizeNoneAttr(
> > + D, Attr.getRange(), Attr.getAttributeSpellingListIndex()))
> > + D->addAttr(Optnone);
> > }
> >
> > static void handleGlobalAttr(Sema &S, Decl *D, const AttributeList
> &Attr) {
> > Index: test/SemaCXX/attr-optnone.cpp
> > ===================================================================
> > --- test/SemaCXX/attr-optnone.cpp
> > +++ test/SemaCXX/attr-optnone.cpp
> > @@ -3,17 +3,17 @@
> > int foo() __attribute__((optnone));
> > int bar() __attribute__((optnone)) __attribute__((noinline));
> >
> > -int baz() __attribute__((always_inline)) __attribute__((optnone)); //
> expected-error{{'always_inline' and 'optnone' attributes are not
> compatible}}
> > -int quz() __attribute__((optnone)) __attribute__((always_inline)); //
> expected-error{{'optnone' and 'always_inline' attributes are not
> compatible}}
> > +int baz() __attribute__((always_inline)) __attribute__((optnone)); //
> expected-warning{{'always_inline' attribute ignored}} expected-
> note{{conflicting attribute is here}}
> > +int quz() __attribute__((optnone)) __attribute__((always_inline)); //
> expected-warning{{'always_inline' attribute ignored}} expected-
> note{{conflicting attribute is here}}
> >
> > __attribute__((always_inline)) int baz1(); // expected-
> warning{{'always_inline' attribute ignored}}
> > __attribute__((optnone)) int baz1() { return 1; } // expected-
> note{{conflicting attribute is here}}
> >
> > __attribute__((optnone)) int quz1(); // expected-note{{conflicting
> attribute is here}}
> > __attribute__((always_inline)) int quz1() { return 1; } // expected-
> warning{{'always_inline' attribute ignored}}
> >
> > -int bay() __attribute__((minsize)) __attribute__((optnone)); //
> expected-error{{'minsize' and 'optnone' attributes are not compatible}}
> > -int quy() __attribute__((optnone)) __attribute__((minsize)); //
> expected-error{{'optnone' and 'minsize' attributes are not compatible}}
> > +int bay() __attribute__((minsize)) __attribute__((optnone)); //
> expected-warning{{'minsize' attribute ignored}} expected-
> note{{conflicting}}
> > +int quy() __attribute__((optnone)) __attribute__((minsize)); //
> expected-warning{{'minsize' attribute ignored}} expected-
> note{{conflicting}}
> >
> > __attribute__((minsize)) int bay1(); // expected-warning{{'minsize'
> attribute ignored}}
> > __attribute__((optnone)) int bay1() { return 1; } // expected-
> note{{conflicting attribute is here}}
> > @@ -27,8 +27,13 @@
> > __attribute__((optnone)) // expected-note 2 {{conflicting}}
> > void bay2() {}
> >
> > -__forceinline __attribute__((optnone)) int bax(); // expected-
> error{{'__forceinline' and 'optnone' attributes are not compatible}}
> > -__attribute__((optnone)) __forceinline int qux(); // expected-
> error{{'optnone' and '__forceinline' attributes are not compatible}}
> > +__forceinline __attribute__((optnone)) int bax(); // expected-
> warning{{'__forceinline' attribute ignored}} expected-note{{conflicting}}
> > +__attribute__((optnone)) __forceinline int qux(); // expected-
> warning{{'__forceinline' attribute ignored}} expected-note{{conflicting}}
> > +
> > +__forceinline int bax2(); // expected-warning{{'__forceinline'
> attribute ignored}}
> > +__attribute__((optnone)) int bax2() { return 1; } // expected-
> note{{conflicting}}
> > +__attribute__((optnone)) int qux2(); // expected-note{{conflicting}}
> > +__forceinline int qux2() { return 1; } // expected-
> warning{{'__forceinline' attribute ignored}}
> >
> > int globalVar __attribute__((optnone)); // expected-warning{{'optnone'
> attribute only applies to functions}}
> >
> > @@ -50,8 +55,8 @@
> > [[clang::optnone]]
> > int bar2() __attribute__((noinline));
> >
> > -[[clang::optnone]]
> > -int baz2() __attribute__((always_inline)); // expected-
> error{{'always_inline' and 'optnone' attributes are not compatible}}
> > +[[clang::optnone]] // expected-note {{conflicting}}
> > +int baz2() __attribute__((always_inline)); // expected-
> warning{{'always_inline' attribute ignored}}
> >
> > [[clang::optnone]] int globalVar2; //expected-warning{{'optnone'
> attribute only applies to functions}}
> >
> >
>
> ~Aaron
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
More information about the cfe-commits
mailing list