[PATCH] D42457: [analyzer] Don't communicate evaluation failures through memregion hierarchy.

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov added inline comments.


================
Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h:570
 
+  struct EvalCallFlags {
+    bool IsConstructorWithImproperlyModeledTargetRegion : 1;
----------------
Those are not really flags. Perhaps options?


================
Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h:572
+    bool IsConstructorWithImproperlyModeledTargetRegion : 1;
+    bool IsArrayConstructorOrDestructor : 1;
+  };
----------------
OK my C++ knowledge is weak here.
 What happens if you don't initialize those at the callsite and then read? Wouldn't be safer to set them both to false in the declaration?


================
Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h:661
+  /// When the lookahead fails, a temporary region is returned, and a flag is
+  /// set in \p Flags.
   const MemRegion *getRegionForConstructedObject(const CXXConstructExpr *CE,
----------------
Which flag?


================
Comment at: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:92
 ///
 /// If the type is not an array type at all, the original value is returned.
 static SVal makeZeroElementRegion(ProgramStateRef State, SVal LValue,
----------------
Doxygen comment for out-parameter? E.g. it's non-obvious that it only sets the flag to true and does not touch it otherwise.


================
Comment at: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:94
 static SVal makeZeroElementRegion(ProgramStateRef State, SVal LValue,
-                                  QualType &Ty) {
+                                  QualType &Ty, bool &IsIndeedAnArray) {
   SValBuilder &SVB = State->getStateManager().getSValBuilder();
----------------
Perhaps `IsArray` will also do?


================
Comment at: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:142
+            bool IsArray = false;
+            LValue = makeZeroElementRegion(State, LValue, Ty, IsArray);
+            if (IsArray)
----------------
Or I suppose you could do `makeZeroElementRegion(..., &Flags.IsArrayConstructorOrDestructor)` to express the intent


================
Comment at: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:171
+      bool IsArray = false;
+      FieldVal = makeZeroElementRegion(State, FieldVal, Ty, IsArray);
+      if (IsArray)
----------------
Same as previous comment, I suppose you could write into `IsArrayConstructorOrDestructor` directly.


================
Comment at: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:451
+  if (IsArray)
+    Flags.IsArrayConstructorOrDestructor = true;
+
----------------
Again perhaps write into `Flags.Is...` directly?


Repository:
  rC Clang

https://reviews.llvm.org/D42457