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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
