[PATCH] D80366: [Analyzer] Add `getReturnValueUnderConstruction()` to `CallEvent`

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.


================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:435
+  /// If the call returns a C++ record type then the region of its return value
+  // can be retrieved from its construction context.
+  Optional<SVal> getReturnValueUnderConstruction(unsigned BlockCount) const;
----------------
balazske wrote:
> A `/` is missing (or too much of `/` above?).
> It looks like that the comment tells something about how the function works, not what it does. Or not? (The function works by retrieving the construction context and something from it, but that seems to be the return value, not a region. This is why this comment can be confusing.) This belongs better to the implementation, not to the documentation part. The documentation could contain something about how and when the function can be used to get a non-null value.
+1

I'd expect a description about what it does and only then the details.


================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h:689
 
+  /// Update the program state with all the path-sensitive information
+  /// that's necessary to perform construction of an object with a given
----------------
Why was it necessary to move this prototype? Is this hunk related at all?


================
Comment at: clang/lib/StaticAnalyzer/Core/CallEvent.cpp:575
+  std::tie(State, RetVal) =
+    static_cast<ExprEngine*>(&Engine)->handleConstructionContext(getOriginExpr(),
+                                                         getState(),
----------------
baloghadamsoftware wrote:
> This is extremely ugly and was one of the reasons I originally did not reuse `handleConstructionContext()`. What should I do here? Create a pure virtual `handleConstructionContext()` into `SubEngine`? Or somehow make `handleConstructionContext` static? Is there maybe a more proper way to access `ExprEngine` from `CallEvent`?
Seems like `SubEngine` has only one descendant: `ExprEngine`. Consequently `SubEngine` is a false interface that is never ever used polymorphically, perhaps we could just get rid of that? I mean what if we used `ExprEngine` everywhere and scraped the `SubEngine` class? `SubEngine` seems to be a superfluous legacy to me (I can be wrong of course).


================
Comment at: clang/unittests/StaticAnalyzer/TestReturnValueUnderConstruction.cpp:29
+    Optional<SVal> RetVal = Call.getReturnValueUnderConstruction(0);
+    assert(RetVal);
+    assert(RetVal->getAsRegion());
----------------
I think it would make the tests cleaner if we had some member variables set here and then in the test function (`addTestReturnValueUnderConstructionChecker`) we had the expectations on those variables.
Right now, if we face an assertion it kills the whole unit test suite and it is not obvious why that happened.


================
Comment at: clang/unittests/StaticAnalyzer/TestReturnValueUnderConstruction.cpp:47
+  EXPECT_TRUE(runCheckerOnCode<addTestReturnValueUnderConstructionChecker>(
+                  "class C { public: C(int nn): n(nn) {} virtual ~C() {} "
+                  "          private: int n; };"
----------------
Please clang-format test code as well! You may also use raw string literals. Please be precise with test code too.


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

https://reviews.llvm.org/D80366