xazax.hun added inline comments.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:408
+ auto makeSValFor = [&C, &State, this](const Expr *E, SVal S) -> SVal {
+ if (S.isZeroConstant()) {
+ return C.getSValBuilder().makeZeroVal(E->getType());
----------------
Do we want to create a new SVal in this case? Why returning `S` is insufficient?
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:416
+ QualType Type = getInnerPointerType(C, E->getType()->getAsCXXRecordDecl());
+ std::tie(Val, State) = retrieveOrConjureInnerPtrVal(Reg, E, Type, C);
+ return Val;
----------------
While this is smart (overwriting the state here), I think this is hard to
follow. I'd prefer this lambda returning a pair.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:435
+ ProgramStateRef TrueState, FalseState;
+ std::tie(TrueState, FalseState) =
+ State->assume(*RetVal.getAs<DefinedOrUnknownSVal>());
----------------
`assume` might not return a state (when the analyzer is smart enough to figure
out one path is infeasible). While your code would probably work as is, as far
as I remember we usually try to not call addTransition with `null` states.
================
Comment at: clang/test/Analysis/smart-ptr.cpp:484
+ clang_analyzer_eval(nullPtr != nullptr); // expected-warning{{FALSE}}
+ clang_analyzer_eval(nullptr <= unknownPtr); // expected-warning{{TRUE}}
+}
----------------
I do not see test cases where the path is actually split. Would you add some?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D104616/new/
https://reviews.llvm.org/D104616
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
