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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
