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

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:176
+  const LocationContext *LCtx = C.getPredecessor()->getLocationContext();
+  ConstraintManager &CM = C.getConstraintManager();
+  auto *CE = dyn_cast_or_null<CallExpr>(Call.getOriginExpr());
----------------
NoQ wrote:
> balazske wrote:
> > baloghadamsoftware wrote:
> > > The four lines above are typical cases for using auto, since the type is duplicated in the line. See: https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable
> > This is not totally clear if we can trust the pattern that at `auto X = Y.getXxx()` type of `X` will be `Xxx`.
> @balazske +1. I'm personally in favor of using `auto` here, but the community's stance is to use `auto` only for `dyn_cast`/`getAs` etc. calls where type is explicitly spelled out, and also for iterators where the type is going to be too long and annoying to spell out (but in the latter case i'm suddenly in favor of spelling it out, as i always get confused about how many `*`s do i need to add in order to properly dereference whatever's in there).
`SValBuilder` is explicitly spelled out here, also `DefinedSVal` below, which comes from a `getAs()`. Btw, is it right to call the actual variable `SValBuilder` as well? I would never do that. I is usually called `SVB`.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:189-190
+  // Check if error was generated.
+  if (C.isDifferent())
+    return;
+
----------------
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.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:69
       {{"fclose", 1}, &StreamChecker::evalFclose},
+      {{"freopen", 3}, &StreamChecker::evalFreopen},
       {{"fread", 4},
----------------
I would move this `freopen` line and everything else (`evalFreopen` header and body) directly after `freopen`. To me it seems more logical.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69948