[PATCH] D76590: [Analyzer] Model `empty()` member function of containers
Kristóf Umann via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Sep 7 03:19:15 PDT 2020
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
More information about the cfe-commits
mailing list