https://github.com/jpjepko created https://github.com/llvm/llvm-project/pull/234396
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 >From 4de88df02542d1191dd30b8a398d66b472b8e47d Mon Sep 17 00:00:00 2001 From: John Paul Jepko <[email protected]> Date: Wed, 21 Oct 2026 20:35:18 +0200 Subject: [PATCH] Improve what key is used for RefsMinusAssignments map Create a key getter function used to index RefsMinusAssignments, which needs special handling for static data members of class templates with instantiated uses. --- clang/include/clang/Sema/Sema.h | 8 +++ clang/lib/Sema/SemaDecl.cpp | 12 +++- clang/lib/Sema/SemaExpr.cpp | 12 ++-- clang/lib/Sema/SemaExprCXX.cpp | 2 +- .../warn-unused-but-set-static-global.cpp | 68 +++++++++++++++++++ 5 files changed, 91 insertions(+), 11 deletions(-) diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h index 9082042c48502d..71b691f230f6d9 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 21e279ec2000c3..9727f7daf44396 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 57cd6d1aa0483e..c818a077bdccd4 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 2d3ba9c45a5ede..7ce1851fb5f281 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 e0354848bdd99a..50bd493322b571 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; +} _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
