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. 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<mailto: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<mailto: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
