[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