llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang Author: John Paul Jepko (jpjepko) <details> <summary>Changes</summary> Warning `-Wunused-but-set-global` (#<!-- -->178342 and #<!-- -->188291) has a false positive when a static data member of a class template (in an anonymous namespace) is set inside the template but read via an instantiation (introduced in commit fd11cf430e5a). The code uses canonical decls to differentiate variables (the key for the hashmap `RefsMinusAssignments`), which in this case makes the instantiation's member appear separately than the template's member, so because they are tracked separately, the read never 'offsets' the write. Fix that error by tracking the template's member and its instantiations with the same key (the canonical decl of the instantiation pattern). As a result, every use of a member in the template or through any implicit/explicit instantiation shares the same count. Explicit specializations remain tracked separately, since they have no instantiation pattern. A consequence of this approach is that members with a type dependent on a template parameter, and a member accessed only via an instantiation, no longer warn. Fixes #<!-- -->231652 Assisted-by: claude-opus-5.5 --- Full diff: https://github.com/llvm/llvm-project/pull/234396.diff 5 Files Affected: - (modified) clang/include/clang/Sema/Sema.h (+8) - (modified) clang/lib/Sema/SemaDecl.cpp (+10-2) - (modified) clang/lib/Sema/SemaExpr.cpp (+4-8) - (modified) clang/lib/Sema/SemaExprCXX.cpp (+1-1) - (modified) clang/test/Sema/warn-unused-but-set-static-global.cpp (+68) ``````````diff diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h index 9082042c48502..71b691f230f6d 100644 --- a/clang/include/clang/Sema/Sema.h +++ b/clang/include/clang/Sema/Sema.h @@ -7100,6 +7100,14 @@ class Sema final : public SemaBase { ExpressionEvaluationContextRecord::ExpressionKind::EK_AttrArgument; } + /// Get the key used for RefsMinusAssignments, the canonical decl. Use the + /// canonical decl so in-class declarations and out-of-class definitions of + /// static data members in anonymous namespaces are tracked as a single entry. + /// + /// Handle the special case of an instantiated static data member by returning + /// the canonical decl of its instantiation pattern. + static const VarDecl *getUnusedButSetKey(const VarDecl *VD); + /// Increment when we find a reference; decrement when we find an ignored /// assignment. Ultimately the value is 0 if every reference is an ignored /// assignment. diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp index 21e279ec2000c..9727f7daf4439 100644 --- a/clang/lib/Sema/SemaDecl.cpp +++ b/clang/lib/Sema/SemaDecl.cpp @@ -2226,6 +2226,14 @@ void Sema::DiagnoseUnusedDecl(const NamedDecl *D, DiagReceiverTy DiagReceiver) { DiagReceiver(DiagLoc, PDiag(DiagID) << D << Hint << SourceRange(DiagLoc)); } +const VarDecl *Sema::getUnusedButSetKey(const VarDecl *VD) { + VD = VD->getCanonicalDecl(); + if (VD->isFileVarDecl()) + if (const VarDecl *P = VD->getTemplateInstantiationPattern()) + return P->getCanonicalDecl(); + return VD; +} + void Sema::DiagnoseUnusedButSetDecl(const VarDecl *VD, DiagReceiverTy DiagReceiver) { // If it's not referenced, it can't be set. If it has the Cleanup attribute, @@ -2269,7 +2277,7 @@ void Sema::DiagnoseUnusedButSetDecl(const VarDecl *VD, if (VD->hasAttr<ObjCPreciseLifetimeAttr>() && Ty->isObjCObjectPointerType()) return; - auto iter = RefsMinusAssignments.find(VD->getCanonicalDecl()); + auto iter = RefsMinusAssignments.find(getUnusedButSetKey(VD)); if (iter == RefsMinusAssignments.end()) return; @@ -2356,7 +2364,7 @@ void Sema::ActOnPopScope(SourceLocation Loc, Scope *S) { if (auto *VD = dyn_cast<VarDecl>(D); VD && !VD->isInternalLinkageFileVar()) { DiagnoseUnusedButSetDecl(VD, addDiag); - RefsMinusAssignments.erase(VD->getCanonicalDecl()); + RefsMinusAssignments.erase(getUnusedButSetKey(VD)); } } diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp index 57cd6d1aa0483..c818a077bdccd 100644 --- a/clang/lib/Sema/SemaExpr.cpp +++ b/clang/lib/Sema/SemaExpr.cpp @@ -20963,14 +20963,10 @@ static void DoMarkVarDeclReferenced( Var->mightBeUsableInConstantExpressions(SemaRef.Context); // Only track variables with internal linkage or local scope. - // Use canonical decl so in-class declarations and out-of-class definitions - // of static data members in anonymous namespaces are tracked as a single - // entry. - const VarDecl *CanonVar = Var->getCanonicalDecl(); - if ((CanonVar->isLocalVarDeclOrParm() || - CanonVar->isInternalLinkageFileVar()) && - !CanonVar->hasExternalStorage()) { - RefsMinusAssignments.insert({CanonVar, 0}).first->getSecond()++; + const VarDecl *Key = Sema::getUnusedButSetKey(Var); + if ((Key->isLocalVarDeclOrParm() || Key->isInternalLinkageFileVar()) && + !Key->hasExternalStorage()) { + RefsMinusAssignments.insert({Key, 0}).first->getSecond()++; } // C++20 [expr.const]p12: diff --git a/clang/lib/Sema/SemaExprCXX.cpp b/clang/lib/Sema/SemaExprCXX.cpp index 2d3ba9c45a5ed..7ce1851fb5f28 100644 --- a/clang/lib/Sema/SemaExprCXX.cpp +++ b/clang/lib/Sema/SemaExprCXX.cpp @@ -7648,7 +7648,7 @@ static void MaybeDecrementCount( if ((IsCompoundAssign || isIncrementDecrementUnaryOp) && VD->getType().isVolatileQualified()) return; - auto iter = RefsMinusAssignments.find(VD->getCanonicalDecl()); + auto iter = RefsMinusAssignments.find(Sema::getUnusedButSetKey(VD)); if (iter == RefsMinusAssignments.end()) return; iter->getSecond()--; diff --git a/clang/test/Sema/warn-unused-but-set-static-global.cpp b/clang/test/Sema/warn-unused-but-set-static-global.cpp index e0354848bdd99..50bd493322b57 100644 --- a/clang/test/Sema/warn-unused-but-set-static-global.cpp +++ b/clang/test/Sema/warn-unused-but-set-static-global.cpp @@ -165,3 +165,71 @@ void f9() { ExtSetInMethod e; e.set(); } + +// Regression tests for static data members of class templates in an +// anonymous namespace. namespace. Since every instantiation of a member are +// tracked as one, a read anywhere in the source counts as a use. +namespace { + // Set in the template and read through an instantiation. + template <int N> struct TmplCounted { + static int live; + TmplCounted() { ++live; } + }; + template <int N> int TmplCounted<N>::live = 0; + + // Read only in an uninstantiated member function. + template <int N> struct TmplUninstReader { + static int x; + void set() { x = 1; } + int get() { return x; } + }; + template <int N> int TmplUninstReader<N>::x = 0; + + // Set but never read in any of its instantiations, but diagnosed only once. + template <int N> struct TmplSetOnly { + static int x; // expected-warning {{variable 'x' set but not used}} + void set() { x = 1; } + }; + template <int N> int TmplSetOnly<N>::x = 0; + + // Explicit specializations get tracked separately from the template. + template <int N> struct TmplExplicit { + static int x; + void set() { x = 1; } + }; + template <int N> int TmplExplicit<N>::x = 0; + template <> int TmplExplicit<1>::x = 1; + template <> int TmplExplicit<2>::x = 2; // expected-warning {{variable 'x' set but not used}} +} + +int f10() { + TmplCounted<0> c; + (void)c; + TmplUninstReader<0>{}.set(); + TmplSetOnly<0>{}.set(); + TmplSetOnly<1>{}.set(); + TmplSetOnly<2>{}.set(); + TmplExplicit<0>{}.set(); + TmplExplicit<1>{}.set(); + TmplExplicit<2>{}.set(); + return TmplCounted<0>::live + TmplExplicit<0>::x + TmplExplicit<1>::x; +} + +// FIXME: These are truly unused but do not warn. +namespace { + template <class T> struct TmplDepType { + static T x; + void set() { x = T(); } + }; + template <class T> T TmplDepType<T>::x; + + template <int N> struct TmplSetViaInst { + static int x; + }; + template <int N> int TmplSetViaInst<N>::x = 0; +} + +void f11() { + TmplDepType<int>{}.set(); + TmplSetViaInst<0>::x = 1; +} `````````` </details> https://github.com/llvm/llvm-project/pull/234396 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
