balazske marked an inline comment as done.
balazske added inline comments.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:504
+
+ OrigStdin = findStdStream("stdin", C);
+ OrigStdout = findStdStream("stdout", C);
----------------
steakhal wrote:
> balazske wrote:
> > steakhal wrote:
> > > martong wrote:
> > > > We should be careful, to cache the results (either here, or deeper in
> > > > the call stack).
> > > > I mean, we certainly don't want to do a lookup of "stdin" every time a
> > > > function is evaluated. We should do this lazily, only once.
> > > I agree. We should do this only for the first top-level function, instead
> > > of doing this for every top-level function.
> > I am not sure if the `SymbolRef` values are safe to store between top-level
> > function analyses. The `FILE` type and std stream declarations are safe to
> > cache, but the SymbolRef values of these variables probably not.
> I think it should be safe. But place there an assert and run the test suite.
> If it won't trigger, that means that we have high confidentiality that this
> is safe. I know it's not 100%, but pretty close.
I tried to check if these `SymbolRef`'s are the same at `checkBeginFunction`
after initialized once. The assertion for equality failed sometimes, or other
crash happened when `dump` was called on the value. So it looks not safe to
cache these. And should we add assumptions about that these `StdinSym`,
`StdoutSym`, `StderrSym` are not equal to each other?
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:555
+ C.getSValBuilder().makeSymbolVal(StdStream),
+ C.getASTContext().getLogicalOperationType());
+ Optional<DefinedOrUnknownSVal> NotEqDef =
----------------
steakhal wrote:
> `SValBuilder::getConditionType()`, oh they are the same under the hood. We
> should still probably prefer this one instead.
> It might worth hoisting `C.getSValBuilder()` to a local variable though.
> The symbol for `StdStream` also deserves a separate variable IMO.
This lambda is possible to improve.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D106644/new/
https://reviews.llvm.org/D106644
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
