vsavchenko added inline comments.
================
Comment at: clang/lib/StaticAnalyzer/Core/CallEvent.cpp:552
+
+ Index = StackFrame->getIndex();
+
----------------
Szelethus wrote:
> baloghadamsoftware wrote:
> > Szelethus wrote:
> > > This mustn't be serious. `StackFrameContext::getIndex()` has **no
> > > comments** at all! So it is an index to the element within a `CFGBlock`
> > > to which it also stores a field for??? Ugh. Shouldn't we just have a
> > > `getCallSiteCFGElement` or something? Especially since we have
> > > `CFGElementRef`s now.
> > >
> > > Would you be so nice to hunt this down please? :)
> > Do you mean a new `StackFrameContext::getCFGElement()` member function? I
> > agree. I can do it, however this technology where we get the block and the
> > index separately is used at many places in the code, then it would be
> > appropriate to refactor all these places. Even wrose is the backward
> > direction, where we look for the CFG element of a statement: we find the
> > block from `CFGStmtMap` and then we search for the index **liearly**,
> > instrad of storing it in the map as well!!!
> Nasty. Yeah, I mean not to burden you work refactoring any more then you feel
> like doing it, but simply adding a `StackFrameContext::getCFGElement()`
> method and using it in this patch would be nice enough, and some comments to
> `getIndex()`. But this obviously isn't a high prio issue.
+1
I do believe that clearer interface functions and better segregation of
different functionalities will make it much easier for us in the future
================
Comment at: clang/lib/StaticAnalyzer/Core/CallEvent.cpp:553
+
+ if(const auto Ctor = (*Block)[Index].getAs<CFGConstructor>()) {
+ return Ctor->getConstructionContext();
----------------
nit: space after `if`
And I would also prefer putting `const auto *Ctor`
================
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:247-249
+ Optional<SVal> ExistingVal = getObjectUnderConstruction(State, CE, LCtx);
+ if (ExistingVal.hasValue())
+ return std::make_pair(State, *ExistingVal);
----------------
Maybe here (and further) we can use a widespread pattern of `bool`-castable
things declared in the `if` condition (like you do with `dyn_casts`):
```
if (Optional<SVal> ExistingVal = getObjectUnderConstruction(State, CE, LCtx))
return std::make_pair(State, *ExistingVal);
```
================
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:345-360
+ if (const auto *CE = dyn_cast<CallExpr>(E)) {
+ Optional<SVal> ExistingVal =
+ getObjectUnderConstruction(State, {CE, Idx}, LCtx);
+ if (ExistingVal.hasValue())
+ return std::make_pair(State, *ExistingVal);
+ } else if (const auto *CCE = dyn_cast<CXXConstructExpr>(E)) {
+ Optional<SVal> ExistingVal =
----------------
It seems like a code duplication to me.
First of all we can first compose a `ConstructionContextItem` from either `CE`,
`CCE`, or `ME` and then call for `getObjectUnderConstruction`.
Additionally, I think we can hide even that by introducing an overload for
`getObjectUnderConstruction` that takes an expression and an index.
================
Comment at:
clang/unittests/StaticAnalyzer/TestReturnValueUnderConstruction.cpp:24
+public:
+ void checkPostCall(const CallEvent &Call, CheckerContext &C) const {
+ if (!Call.getOriginExpr())
----------------
martong wrote:
> I assume this tests this call expression: `returnC(1)` . But this is
> absolutely not trivial from the test code, could you please make this cleaner?
I think that this function, the meat and bones of the test, should be properly
commented and state explicitly what are the assumption and what is the expected
outcome.
================
Comment at:
clang/unittests/StaticAnalyzer/TestReturnValueUnderConstruction.cpp:29
+ Optional<SVal> RetVal = Call.getReturnValueUnderConstruction(0);
+ assert(RetVal);
+ assert(RetVal->getAsRegion());
----------------
martong wrote:
> 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.
Why not `gtest`s assertions? Those will also interrupt the execution, but will
be much clearer in the output.
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
