[PATCH] D69948: [Checkers] Added support for freopen to 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:189-190
+  // Check if error was generated.
+  if (C.isDifferent())
+    return;
+
----------------
baloghadamsoftware wrote:
> balazske wrote:
> > NoQ wrote:
> > > That's actually one more good reason to move all `checkNullStream` to `PreCall`: less spaghetti. The same checker should feel free to subscribe for the same function in multiple different callbacks and do different things in them.
> > > 
> > > Rule of a thumb (i'm not sure if it's //always// applicable but it's applicable to most "typical" checkers even if they aren't currently written this way):
> > > 1. Pre-condition checking goes in PreCall;
> > > 2. Modeling that affects everybody goes in evalCall;
> > > 3. Checker's internal bookkeeping goes in PostCall.
> > > 
> > Precondition (check for NULL stream) can go into PreCall. Probably the bookkeeping task is more difficult to do, if the state is split we have already the different states available in `evalCall` but need to check the state in `postCall`.
> @NoQ +1 I completely agree: All precondition checks in this checker should be moved into `PreCall`. I would even go further: separate the checker into a `StreamModeling` which models the calls using `evalCall` and `StreamChecker` which does the checks. However, these NFCs should be done in subsequent patches, not this one.
Does the setting of new state (`State->set<StreamMap>`) belong to the "modeling" (`evalCall`) or "checker" (in the `postCall`) part?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69948