[PATCH] D126534: [analyzer] Deadstore static analysis: Fix false positive on C++17 assignments

[Fred Tingaud via Phabricator via cfe-commits]

frederic-tingaud-sonarsource updated this revision to Diff 436766.
frederic-tingaud-sonarsource added a comment.

Update diagnostic location to highlight the whole content of the initialization.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126534/new/

https://reviews.llvm.org/D126534

Files:
  clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp
  clang/test/Analysis/dead-stores.cpp


Index: clang/test/Analysis/dead-stores.cpp
===================================================================
--- clang/test/Analysis/dead-stores.cpp
+++ clang/test/Analysis/dead-stores.cpp
@@ -11,6 +11,10 @@
 // RUN: %clang_analyze_cc1 -fcxx-exceptions -fexceptions -fblocks -std=c++11    \
 // RUN:  -analyzer-checker=deadcode.DeadStores -Wno-unreachable-code            \
 // RUN:  -verify=non-nested,nested %s
+//
+// RUN: %clang_analyze_cc1 -fcxx-exceptions -fexceptions -fblocks -std=c++17    \
+// RUN:  -analyzer-checker=deadcode.DeadStores -Wno-unreachable-code            \
+// RUN:  -verify=non-nested,nested %s
 
 //===----------------------------------------------------------------------===//
 // Basic dead store checking (but in C++ mode).
@@ -52,6 +56,18 @@
   return x;
 }
 
+class TestConstructor {
+public:
+  TestConstructor(int &y);
+};
+void copy(int x) {
+  // All these calls might have side effects in the opaque constructor
+  TestConstructor tc1 = x;                    // no-warning potential side effects
+  TestConstructor tc2 = TestConstructor(x);   // no-warning potential side effects
+  TestConstructor tc3 = (TestConstructor(x)); // no-warning potential side effects
+  TestConstructor tc4 = (TestConstructor)(x); // no-warning potential side effects
+}
+
 //===----------------------------------------------------------------------===//
 // Dead store checking involving CXXTemporaryExprs
 //===----------------------------------------------------------------------===//
Index: clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp
@@ -103,8 +103,8 @@
 static const Expr *
 LookThroughTransitiveAssignmentsAndCommaOperators(const Expr *Ex) {
   while (Ex) {
-    const BinaryOperator *BO =
-      dyn_cast<BinaryOperator>(Ex->IgnoreParenCasts());
+    Ex = Ex->IgnoreParenCasts();
+    const BinaryOperator *BO = dyn_cast<BinaryOperator>(Ex);
     if (!BO)
       break;
     BinaryOperatorKind Op = BO->getOpcode();
@@ -331,8 +331,7 @@
           // Special case: check for assigning null to a pointer.
           //  This is a common form of defensive programming.
           const Expr *RHS =
-            LookThroughTransitiveAssignmentsAndCommaOperators(B->getRHS());
-          RHS = RHS->IgnoreParenCasts();
+              LookThroughTransitiveAssignmentsAndCommaOperators(B->getRHS());
 
           QualType T = VD->getType();
           if (T.isVolatileQualified())
@@ -415,8 +414,7 @@
               if (isConstant(E))
                 return;
 
-              if (const DeclRefExpr *DRE =
-                      dyn_cast<DeclRefExpr>(E->IgnoreParenCasts()))
+              if (const DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(E))
                 if (const VarDecl *VD = dyn_cast<VarDecl>(DRE->getDecl())) {
                   // Special case: check for initialization from constant
                   //  variables.
@@ -438,7 +436,7 @@
 
               PathDiagnosticLocation Loc =
                 PathDiagnosticLocation::create(V, BR.getSourceManager());
-              Report(V, DeadInit, Loc, E->getSourceRange());
+              Report(V, DeadInit, Loc, V->getInit()->getSourceRange());
             }
           }
         }
@@ -450,8 +448,9 @@
   bool isConstant(const InitListExpr *Candidate) const {
     // We consider init list to be constant if each member of the list can be
     // interpreted as constant.
-    return llvm::all_of(Candidate->inits(),
-                        [this](const Expr *Init) { return isConstant(Init); });
+    return llvm::all_of(Candidate->inits(), [this](const Expr *Init) {
+      return isConstant(Init->IgnoreParenCasts());
+    });
   }
 
   /// Return true if the given expression can be interpreted as constant
@@ -461,7 +460,7 @@
       return true;
 
     // We should also allow defensive initialization of structs, i.e. { 0 }
-    if (const auto *ILE = dyn_cast<InitListExpr>(E->IgnoreParenCasts())) {
+    if (const auto *ILE = dyn_cast<InitListExpr>(E)) {
       return isConstant(ILE);
     }
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D126534.436766.patch
Type: text/x-patch
Size: 4190 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20220614/4bbb10cf/attachment.bin>