On Thu, Apr 28, 2016 at 4:46 PM, Robinson, Paul <paul.robin...@sony.com> wrote:
> Generally tests test something other than "this program doesn't crash" - > should it test that we apply the attribute correctly? (either via ast dump, > or checking the resulting DWARF doesn't have debug info on the relevant > entity) > > Or is this not actually a new/separate codepath? In which case do we > really need the test? > > > > It's a –verify test which is about diagnostics, rather than not-crashing. > It parallels line 3 of Sema/attr-nodebug.c, which verifies the attribute > can be applied to a function. Without this test, you could remove > "ObjCMethod" from the Subjects line and no test would fail. I put > "ObjCMethod" on the Subjects line because the hand-coded condition used > "isFunctionOrMethod" which permits Objective-C methods. The new test > passes with old and new compilers, demonstrating the NFC claim. > Ah, OK - thanks for the context :) > > > Now, if we decide the 'nodebug' attribute should not apply to Objective-C, > that's fine with me, in which case the new test should verify that a > diagnostic *is* produced. > > > > I am admittedly clueless about Objective-C, are they handled differently > from other functions by the time we get to CGDebugInfo? If there's another > path I should tweak, I'd like to know about it. > > Thanks, > > --paulr > > > > *From:* David Blaikie [mailto:dblai...@gmail.com] > *Sent:* Thursday, April 28, 2016 4:26 PM > *To:* reviews+d19689+public+514682b5314c5...@reviews.llvm.org; Robinson, > Paul > *Cc:* Aaron Ballman; cfe-commits > *Subject:* Re: [PATCH] D19689: Add Subjects to NoDebugAttr [NFC] > > > > LGTM > > > > On Thu, Apr 28, 2016 at 2:10 PM, Paul Robinson via cfe-commits < > cfe-commits@lists.llvm.org> wrote: > > probinson created this revision. > probinson added a reviewer: aaron.ballman. > probinson added a subscriber: cfe-commits. > > The 'nodebug' attribute had hand-coded constraints; replace those with a > Subjects line in Attr.td. > Also add a missing test to verify the attribute is okay on an Objective-C > method. > > http://reviews.llvm.org/D19689 > > Files: > include/clang/Basic/Attr.td > include/clang/Basic/DiagnosticSemaKinds.td > lib/Sema/SemaDeclAttr.cpp > test/Sema/attr-nodebug.c > test/SemaObjC/attr-nodebug.m > > Index: test/SemaObjC/attr-nodebug.m > =================================================================== > --- test/SemaObjC/attr-nodebug.m > +++ test/SemaObjC/attr-nodebug.m > @@ -0,0 +1,5 @@ > +// RUN: %clang_cc1 -fsyntax-only -verify %s > +// expected-no-diagnostics > +@interface NSObject > +- (void)doSomething __attribute__((nodebug)); > +@end > > > > Generally tests test something other than "this program doesn't crash" - > should it test that we apply the attribute correctly? (either via ast dump, > or checking the resulting DWARF doesn't have debug info on the relevant > entity) > > Or is this not actually a new/separate codepath? In which case do we > really need the test? > > > > Index: test/Sema/attr-nodebug.c > =================================================================== > --- test/Sema/attr-nodebug.c > +++ test/Sema/attr-nodebug.c > @@ -3,7 +3,7 @@ > int a __attribute__((nodebug)); > > void b() { > - int b __attribute__((nodebug)); // expected-warning {{'nodebug' only > applies to variables with static storage duration and functions}} > + int b __attribute__((nodebug)); // expected-warning {{'nodebug' > attribute only applies to functions and global variables}} > } > > void t1() __attribute__((nodebug)); > Index: lib/Sema/SemaDeclAttr.cpp > =================================================================== > --- lib/Sema/SemaDeclAttr.cpp > +++ lib/Sema/SemaDeclAttr.cpp > @@ -3572,18 +3572,6 @@ > } > > static void handleNoDebugAttr(Sema &S, Decl *D, const AttributeList > &Attr) { > - if (const VarDecl *VD = dyn_cast<VarDecl>(D)) { > - if (!VD->hasGlobalStorage()) > - S.Diag(Attr.getLoc(), > - diag::warn_attribute_requires_functions_or_static_globals) > - << Attr.getName(); > - } else if (!isFunctionOrMethod(D)) { > - S.Diag(Attr.getLoc(), > - diag::warn_attribute_requires_functions_or_static_globals) > - << Attr.getName(); > - return; > - } > - > D->addAttr(::new (S.Context) > NoDebugAttr(Attr.getRange(), S.Context, > Attr.getAttributeSpellingListIndex())); > Index: include/clang/Basic/DiagnosticSemaKinds.td > =================================================================== > --- include/clang/Basic/DiagnosticSemaKinds.td > +++ include/clang/Basic/DiagnosticSemaKinds.td > @@ -2504,9 +2504,6 @@ > def warn_incomplete_encoded_type : Warning< > "encoding of %0 type is incomplete because %1 component has unknown > encoding">, > InGroup<DiagGroup<"encode-type">>; > -def warn_attribute_requires_functions_or_static_globals : Warning< > - "%0 only applies to variables with static storage duration and > functions">, > - InGroup<IgnoredAttributes>; > def warn_gnu_inline_attribute_requires_inline : Warning< > "'gnu_inline' attribute requires function to be marked 'inline'," > " attribute ignored">, > Index: include/clang/Basic/Attr.td > =================================================================== > --- include/clang/Basic/Attr.td > +++ include/clang/Basic/Attr.td > @@ -973,6 +973,8 @@ > > def NoDebug : InheritableAttr { > let Spellings = [GCC<"nodebug">]; > + let Subjects = SubjectList<[FunctionLike, ObjCMethod, GlobalVar], > WarnDiag, > + > "ExpectedFunctionGlobalVarMethodOrProperty">; > let Documentation = [NoDebugDocs]; > } > > > > > _______________________________________________ > cfe-commits mailing list > cfe-commits@lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits > > >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
