[PATCH] D76590: [Analyzer] Model `empty()` member function of containers

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware marked an inline comment as done.
baloghadamsoftware added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp:36
+  void handleAssignment(CheckerContext &C, const Expr *CE, SVal Cont,
+                        Optional<SVal> = None) const;
+
----------------
Szelethus wrote:
> 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?
We discussed with @NoQ a few patches earlier that `UnknownVal` is not a `nullptr`-like value for `SVal`s. That time I changed this bad practice in my code everywhere, only forgot this particular place. I do not think that this deserves its own patch, it is really a tiny thing.


================
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:
> 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?
Actually, both `UnknownVal` and `SymbolVal` containing a `SymbolConjured` without constraints are unknown. The main difference (for us) is that we cannot assign constraints to `UnknownVal` but we can for `SymbolConjured`. This is the reason for replacing it. However, this is not new. We do it in comparison too. The best would be to change the infrastructure to never return `UnknownVal` but conjure a new symbol instead if we known nothing about the return value. Maybe I will put a `FIXME` there. `evalCall()` is not an option, because we do not want to lose the possibility that the implementation of `empty()` is inlined by the engine.


================
Comment at: clang/unittests/StaticAnalyzer/TestReturnValueUnderConstruction.cpp:71
+
+  EXPECT_TRUE(runCheckerOnCode<addTestReturnValueUnderConstructionChecker>(
+      R"(class C {
----------------
Szelethus wrote:
> Did you mean to upload changed to this file from D85351 to this patch as well?
No, I just uploaded the wrong diff here.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76590/new/

https://reviews.llvm.org/D76590