balazske marked 3 inline comments as done.
balazske added inline comments.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:288-290
+ SymbolRef Sym = RetVal.getAsSymbol();
+ stateNotNull = stateNotNull->set<StreamMap>(Sym, StreamState::getOpened());
+ stateNull = stateNull->set<StreamMap>(Sym, StreamState::getOpenFailed());
----------------
Szelethus wrote:
> Aha, so we're no longer checking whether `Sym` is null, because why would we,
> we literally made created it as a symbol a couple lines up. Is it worth
> `assert()`ing though?
Probably better to have the assert.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:296-297
-ProgramStateRef StreamChecker::CheckNullStream(SVal SV, ProgramStateRef state,
- CheckerContext &C) const {
+Optional<ProgramStateRef>
+StreamChecker::CheckNullStream(SVal SV, CheckerContext &C) const {
Optional<DefinedSVal> DV = SV.getAs<DefinedSVal>();
----------------
Szelethus wrote:
> Why is there a need to use an `Optional`? Why not just return a `nullptr`? As
> I saw it, each time we check both whether the optional has a value //and//
> whether the state within it is null.
The idea was that there are 3 kind of return values: Change to a new state, do
not change to new state, or error was found. In the first and last case we
should return `true` from `evalCall` but not if there is no state to change
into and the analysis can proceed in default way.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D67706/new/
https://reviews.llvm.org/D67706
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
