balazske marked 6 inline comments as done. balazske added inline comments.
================ Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:88 struct FnDescription; using FnCheck = std::function<void(const StreamChecker *, const FnDescription *, const CallEvent &, CheckerContext &)>; ---------------- NoQ wrote: > `llvm::function_ref`? `function_ref`'s documentation says: > This class does not own the callable, so it is not in general safe to store a > function_ref. The `FnDescription` stores these functions so it is not safe to use `llvm::function_ref`? ================ Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:366 + + State = State->set<StreamMap>(StreamSym, StreamState::getOpened()); + C.addTransition(State); ---------------- NoQ wrote: > So what exactly happens when you're calling `clearerr` on a closed stream? > Does it actually become opened? Also what happens when you're calling it on > an open-failed stream? The stream gets always in opened and error-free state. The `preDefault` checks that the stream is opened (not closed and not open-failed) and creates error node if not. If this create fails that is a problem case, the stream will be opened after `clearerr`. Should check in the eval functions for closed stream even if the pre-functions (in `preCall` callback) have normally checked this? ================ Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:379-381 + const CallExpr *CE = dyn_cast_or_null<CallExpr>(Call.getOriginExpr()); + if (!CE) + return; ---------------- NoQ wrote: > Maybe we should add a helper function for this, > `CallEvent::getOriginCallExpr()`. That's an otherwise ugly function given > that `CallEvent ` is a class hierarchy and the superclass doesn't need to > know about subclasses, but most users do in fact only care about > call-expressions and concise checker API is important. Same with > `dyn_cast_or_null<FunctionDecl>(Call.getDecl())` that could be written as > `Call.getFunctionDecl()`. Can be done in another revision together with changing every use of `dyn_cast_or_null<CallExpr>(Call.getOriginExpr())` to the new function. ================ Comment at: clang/test/Analysis/stream-error.c:33-34 + int ch = fputc('a', F); + if (ch == EOF) { + // FIXME: fputc not handled by checker yet, should expect TRUE + clang_analyzer_eval(feof(F) || ferror(F)); // expected-warning {{FALSE}} ---------------- NoQ wrote: > Szelethus wrote: > > Sure, we don't, but the bigger issue here is that we wouldn't mark `F` to > > be in EOF even if we did handle it, we would also need to understand that > > `ch == EOF` is the telling sign. But that also ins't in the scope of this > > patch :) > Yeah, that's gonna be fun. If there is a `fputc` call (for example) that is recognized (has recognized stream and no functions "fail") there is already an error and non-error branch created for it. On the error branch the return value of `fputc` would be **EOF** and the stream state is set to error. (The `fputc` should be later modeled by this checker, not done yet.) If the `ch == EOF` is found then to figure out if this `ch` comes from an `fputc` that is called on a stream that is not recognized (it may be function parameter) and then make a "tracked" stream with error state for it, this is another thing. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75682/new/ https://reviews.llvm.org/D75682 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits