balazske marked 22 inline comments as done and an inline comment as not done. balazske added inline comments.
================ Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:90-106 + FnCheck identifyCall(const CallEvent &Call, CheckerContext &C, + const CallExpr *CE) const; + + void evalFopen(CheckerContext &C, const CallExpr *CE) const; + void evalTmpfile(CheckerContext &C, const CallExpr *CE) const; + void evalFclose(CheckerContext &C, const CallExpr *CE) const; + void evalFread(CheckerContext &C, const CallExpr *CE) const; ---------------- NoQ wrote: > balazske wrote: > > NoQ wrote: > > > For most purposes it's more convenient to pass `CallEvent` around. The > > > only moment when you actually need the `CallExpr` is when you're doing > > > the binding for the return value, which happens once, so it's fine to > > > call `.getOriginExpr()` directly at this point. > > The CallExpr is used at many places to get arguments and other data. There > > is a `CallEvent::getArgSVal` that can be used instead but at some places > > CallExpr is used in other ways. I do not see much benefit of change the > > passed `CallExpr` to `CallEvent`. > > at some places CallExpr is used in other ways > > Can we add more convenient getters to `CallEvent` to help with those? 'Cause > `CallEvent` has strictly more information than `CallExpr` (combining > syntactic information with path-sensitive information), it's all about > convenience. > > I also see that `CallExpr` is stored in `StreamState` (which is something you > can't do with a `CallEvent` because it's short-lived) but i suspect that it's > actually never read from there and i doubt that it was the right solution > anyway. I.e., no other checker needs that and i don't have a reason to > believe that `StreamChecker` is special. In the new code `CallEvent` is passed. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:61 + using FnCheck = bool (StreamChecker::*)(const CallEvent &, + CheckerContext &) const; + ---------------- Szelethus wrote: > Charusso wrote: > > I prefer `std::function`, because it is modern. > > ``` > > using StreamCheck = > > std::function<void(const StreamChecker *, const CallEvent &, > > CheckerContext &)>; > > ``` > > I think it is fine with pointers, but at some point we need to modernize > > this. > But its also a lot more expensive. > https://blog.demofox.org/2015/02/25/avoiding-the-performance-hazzards-of-stdfunction/ > > `std::function` is able to wrap lambdas with different captures and all sorts > of things like that, which comes at a cost. Now `std::function` and `std::bind` is used. Probably more expensive but it is called once in a `evalCall`, that should be no problem? ================ Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:99 + Optional<ProgramStateRef> CheckNullStream(SVal SV, CheckerContext &C) const; + Optional<ProgramStateRef> CheckDoubleClose(const CallEvent &Call, + CheckerContext &C) const; ---------------- Charusso wrote: > This `Optional` is not necessary. When the state is changed, you can rely on > `CheckerContext::isDifferent()` to see whether the modeling was succeeded. > Therefore you could revert the `bool` return values as well. `Optional` is not used now. The functions only return if the passed state was modified (if applicable). To detect if error was generated the `isDifferent` is used. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:137 + return *Callback; } ---------------- Charusso wrote: > I would move this tiny `identifyCall()` into `evalCall()` as the purpose of > `evalCall()` is the validation of the `Callback` in this case. `identifyCall` is removed now. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:196 if (ExplodedNode *N = C.generateNonFatalErrorNode(state)) { if (!BT_illegalwhence) ---------------- NoQ wrote: > This is getting pretty messy, given that you've already made a transition in > this function. If you do `addTransition` and then > `generateNonFatalErrorNode`, then you'll get two parallel successor nodes, > but you only need one. You should either delay the transition that happens > immediately after `CheckNullStream`, or chain the two transitions together > sequentially (as opposed to in parallel). `evalFseek` has now a transition to new (non-null stream) state or transition to (non-fatal) error. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67706/new/ https://reviews.llvm.org/D67706 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
