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