Szelethus added a comment.
I'm in favor of most, if not all of the changes, though I will admit that this
patch seems pretty cluttered, you are doing a lot of refactoring under the same
hood. You're moving, adding, removing and changing helper functions and their
invocations. Would be possible to make this patch a bit leaner?
================
Comment at: clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp:36
+ void handleAssignment(CheckerContext &C, const Expr *CE, SVal Cont,
+ Optional<SVal> = None) const;
+
----------------
Hmm, this was changed to an optional, unnamed parameter without docs... Might
be a bit cryptic :) Also, this seems to be orthogonal to the patch, is it not?
Does the modeling of `empty()` change something that affects this function?
================
Comment at: clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp:420
+
+ // We cannot make assumpotions on `UnknownVal`. Let us conjure a symbol
+ // instead.
----------------
assumpotions > assumptions
================
Comment at: clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp:420-427
+ // We cannot make assumpotions on `UnknownVal`. Let us conjure a symbol
+ // instead.
+ if (RetVal.isUnknown()) {
+ auto &SymMgr = C.getSymbolManager();
+ RetVal = nonloc::SymbolVal(SymMgr.conjureSymbol(
+ CE, LCtx, C.getASTContext().BoolTy, C.blockCount()));
+ State = State->BindExpr(CE, LCtx, RetVal);
----------------
Szelethus wrote:
> assumpotions > assumptions
You will have to help me out here -- if the analyzer couldn't return a sensible
symbol, is it okay to just create one? When does `UnknownVal` even happen, does
it ever happen? Also, if we're doing this anyways, wouldn't using `evalCall` be
more appropriate?
================
Comment at:
clang/unittests/StaticAnalyzer/TestReturnValueUnderConstruction.cpp:71
+
+ EXPECT_TRUE(runCheckerOnCode<addTestReturnValueUnderConstructionChecker>(
+ R"(class C {
----------------
Did you mean to upload changed to this file from D85351 to this patch as well?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D76590/new/
https://reviews.llvm.org/D76590
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
