[PATCH] D69948: [Checkers] Added support for freopen to StreamChecker.

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

balazske marked 5 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;
+
----------------
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`.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:219-220
+  if (StreamSym)
+    StateRetNotNull =
+        StateRetNotNull->set<StreamMap>(StreamSym, StreamState::getOpened());
+  C.addTransition(StateRetNotNull);
----------------
NoQ wrote:
> What does really happen when you `freopen` a stream that isn't already open? It sounds like it's doing a `fclose` blindly, so we might as well report a double close in this case, and then we don't need to update the program state because it's already opened(?) Though later we still might want to update the program state if we start keeping track of the filename or the mode.
The documentation says that the stream is closed but error is ignored. So the double close should be no problem. If the file was already open the state should not be changed (the `set` call handles that case and does not generate new state?).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69948