[PATCH] D67706: [clang][analyzer] Using CallDescription in StreamChecker.

[Balázs Kéri via Phabricator via cfe-commits]

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