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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
