[PATCH] D22374: [analyzer] Copy and move constructors - ExprEngine extended for "almost trivial" copy and move constructors

[Gábor Horváth via cfe-commits]

xazax.hun added a comment.

Note that, once a constructor is not available, we will conservatively treat it as nontrivial. This is not the case however for most of the templated code. Since the STL use templates heavily I think this patch is a great step forward in improving the modeling of C++ code.


================
Comment at: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:37
@@ -36,1 +36,3 @@
 
+bool isCopyOrMoveLike(const CXXConstructorDecl *Constr) {
+  if (Constr->isCopyOrMoveConstructor())
----------------
These two functions only used in this translation unit right? Maybe it would be better to make them static.

================
Comment at: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:57
@@ +56,3 @@
+
+  if(ParamRecDecl!=ThisRecDecl)
+    return false;
----------------
nit: need spaced around the operator.

================
Comment at: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:63
@@ +62,3 @@
+
+bool isAlmostTrivial(const CXXMethodDecl *Met) {
+  if (Met->isTrivial())
----------------
Isn't this only applicable to ctors? Maybe you should reflect this in the name or change the type of the parameter accordingly.

================
Comment at: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:67
@@ +66,3 @@
+
+  if(!Met->hasTrivialBody())
+    return false;
----------------
Please document that, in case we do not see the body of a ctor, we treat it conservatively as non trivial. (hasTrivialBody returns false when the body is not available)

================
Comment at: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:88
@@ +87,3 @@
+      if(Initzer->isBaseInitializer() &&
+         Initzer->getBaseClass() == &*Base.getType()) {
+        if(const auto *CtrCall = dyn_cast<CXXConstructExpr>(Initzer->getInit()->IgnoreParenImpCasts())) {
----------------
Maybe you could reduce the indentation if you use "early continue" here.

================
Comment at: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:109
@@ +108,3 @@
+    if(!Field->getType()->isScalarType() &&
+       !Field->getType()->isRecordType())
+      return false;
----------------
What types do we want to exclude and why? It might be a good idea to document them.

================
Comment at: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:112
@@ +111,3 @@
+    bool found = false;
+    for(const auto *Initzer: Constr->inits()) {
+      if(Initzer->isMemberInitializer() && Initzer->getMember() == Field) {
----------------
Instead of the O(n^2) algorithm, it might be better to just iterate through the initializers, and put each field into a llvm small pointer set, or something like that. Afterwards you can iterate through the fields of the struct and check whether each field is inside the set. This might be both more efficient and cleaner.


https://reviews.llvm.org/D22374