Add missing check for access modifier in UnnecessaryMutableChecker; f…

…ix reported location
cms-sw · Dec 20, 2024 · 739976f · 739976f
1 parent 63d4310
commit 739976f
Show file tree

Hide file tree

Showing 2 changed files with 19 additions and 11 deletions.
diff --git a/Utilities/StaticAnalyzers/src/UnnecessaryMutableChecker.cpp b/Utilities/StaticAnalyzers/src/UnnecessaryMutableChecker.cpp
@@ -46,6 +46,15 @@ namespace clangcms {
       if (!Field->isMutable()) {
         continue;
       }
+
+      // == Skip non-private mutables
+      // Public mutable members are flagged by PublicMutableChecker
+      // Private ones can be modified by other classes, so it is bit hard to find if
+      // a `mutable` attribute is not necessary (i.e. not modified in any const method).
+      if (Field->getAccess() != clang::AS_private) {
+        return;
+      }
+
       // == Skip atmoic mutables, these are thread-safe by design ==
       if (support::isStdAtomic(Field)) {
         return;  // Skip if it's a mutable std::atomic
@@ -64,16 +73,17 @@ namespace clangcms {
       if (!isMutableMemberModified(Field, RD)) {
         clang::SourceLocation Loc = Field->getLocation();
         if (Loc.isValid()) {
+          clang::ento::PathDiagnosticLocation FieldLoc(Loc, SM);
           if (!BT) {
             BT = std::make_unique<clang::ento::BugType>(this, "Unnecessarily Mutable Member", "Coding Practices");
           }
-          BR.EmitBasicReport(
-              Field,
-              this,
-              "Useless mutable field",
-              "ConstThreadSafety",
-              "The mutable field '" + Field->getQualifiedNameAsString() + "' is not modified in any const methods",
-              PathLoc);
+          BR.EmitBasicReport(Field,
+                             this,
+                             "Useless mutable field",
+                             "ConstThreadSafety",
+                             "The mutable field '" + Field->getQualifiedNameAsString() +
+                                 "' is not modified in any public const methods",
+                             FieldLoc);
         }
       }
     }

diff --git a/Utilities/StaticAnalyzers/test/reference.log b/Utilities/StaticAnalyzers/test/reference.log
@@ -1,6 +1,5 @@
 src/Utilities/StaticAnalyzers/test/class_checker.cpp:108:5: warning: 1st function call argument is an uninitialized value [core.CallAndMessage]
 src/Utilities/StaticAnalyzers/test/class_checker.cpp:10:1: warning: Non-const variable 'int * g_ptr_static' is static and might be thread-unsafe [threadsafety.GlobalStatic]
-src/Utilities/StaticAnalyzers/test/class_checker.cpp:12:1: warning: The mutable field 'ClassTest::m_pubMutable' is not modified in any const methods [cms.CodeRules.UnnecessaryMutableChecker]
 src/Utilities/StaticAnalyzers/test/class_checker.cpp:161:10: warning: Value stored to 'ip' during its initialization is never read [deadcode.DeadStores]
 src/Utilities/StaticAnalyzers/test/class_checker.cpp:162:10: warning: Value stored to 'cip' during its initialization is never read [deadcode.DeadStores]
 src/Utilities/StaticAnalyzers/test/class_checker.cpp:167:5: warning: Non-const variable 'int & intRef' is static local or static member data and might be thread-unsafe [threadsafety.StaticLocal]
@@ -15,7 +14,6 @@ src/Utilities/StaticAnalyzers/test/global_static.cpp:10:1: warning: Non-const va
 src/Utilities/StaticAnalyzers/test/global_static.cpp:11:1: warning: Non-const variable 'int * g_ptr_static' is static and might be thread-unsafe [threadsafety.GlobalStatic]
 src/Utilities/StaticAnalyzers/test/global_static_edm.cpp:19:1: warning: Non-const variable 'int g_static' is static and might be thread-unsafe [threadsafety.GlobalStatic]
 src/Utilities/StaticAnalyzers/test/global_static_edm.cpp:22:1: warning: Non-const variable 'edm::InputSourcePluginFactory g_static_edm_namespace' is static and might be thread-unsafe [threadsafety.GlobalStatic]
-src/Utilities/StaticAnalyzers/test/mutable_member.cpp:12:1: warning: The mutable field 'Foo::badPublicMutable' is not modified in any const methods [cms.CodeRules.UnnecessaryMutableChecker]
 src/Utilities/StaticAnalyzers/test/mutable_member.cpp:14:41: warning: Calling non-const method 'Bar::someMethod' of mutable member 'Foo::barMutableMember' in a const member function is potentially thread-unsafe [threadsafety.MutableMember]
 src/Utilities/StaticAnalyzers/test/mutable_member.cpp:19:50: warning: Modifying mutable member 'Foo::privateMutable' in const member function is potentially thread-unsafe  [threadsafety.MutableMember]
 src/Utilities/StaticAnalyzers/test/mutable_member.cpp:20:50: warning: Modifying mutable member 'Foo::privateMutable' in const member function is potentially thread-unsafe  [threadsafety.MutableMember]
@@ -27,7 +25,7 @@ src/Utilities/StaticAnalyzers/test/mutable_member.cpp:25:50: warning: Modifying
 src/Utilities/StaticAnalyzers/test/mutable_member.cpp:29:15: warning: Class 'Foo' has publically-accessible mutable member 'Foo::badPublicMutable', this is not allowed [threadsafety.PublicMutableMember]
 src/Utilities/StaticAnalyzers/test/static_local.cpp:8:5: warning: Non-const variable 'int & intRef' is static local or static member data and might be thread-unsafe [threadsafety.StaticLocal]
 src/Utilities/StaticAnalyzers/test/static_local.cpp:9:5: warning: Non-const variable 'int * intPtr' is static local or static member data and might be thread-unsafe [threadsafety.StaticLocal]
+src/Utilities/StaticAnalyzers/test/unnecessary_mutable_member.cpp:11:15: warning: The mutable field 'Foo::m_intMutable' is not modified in any public const methods [cms.CodeRules.UnnecessaryMutableChecker]
+src/Utilities/StaticAnalyzers/test/unnecessary_mutable_member.cpp:12:23: warning: The mutable field 'Foo::m_strMutable' is not modified in any public const methods [cms.CodeRules.UnnecessaryMutableChecker]
 src/Utilities/StaticAnalyzers/test/unnecessary_mutable_member.cpp:18:5: warning: Modifying mutable member 'Bar::m_intMutable' in const member function is potentially thread-unsafe  [threadsafety.MutableMember]
 src/Utilities/StaticAnalyzers/test/unnecessary_mutable_member.cpp:26:57: warning: Modifying mutable member 'Bar::m_strMutable' in const member function is potentially thread-unsafe  [threadsafety.MutableMember]
-src/Utilities/StaticAnalyzers/test/unnecessary_mutable_member.cpp:4:1: warning: The mutable field 'Foo::m_intMutable' is not modified in any const methods [cms.CodeRules.UnnecessaryMutableChecker]
-src/Utilities/StaticAnalyzers/test/unnecessary_mutable_member.cpp:4:1: warning: The mutable field 'Foo::m_strMutable' is not modified in any const methods [cms.CodeRules.UnnecessaryMutableChecker]