[PATCH] D106644: [clang][analyzer] Add standard streams to alpha.unix.Stream checker.

[Balázs Benics via Phabricator via cfe-commits]

steakhal added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ASTLookup.cpp:26
+  // and we have a TypedefDecl with the name 'FILE'.
+  for (Decl *D : LookupRes)
+    if (auto *TD = dyn_cast<TypedefNameDecl>(D))
----------------
`Decl *D` -> `const Decl *D`. Same for the other loop.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:225
+
+  Optional<QualType> FILEType = GetPointer(LookupType("FILE"));
+
----------------
You calculate this 3 times now. I mean it's not a big deal, but we could save this to do it only once.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:227-236
+  for (Decl *D : LookupRes) {
+    D = D->getCanonicalDecl();
+    if (!C.getSourceManager().isInSystemHeader(D->getLocation()))
+      continue;
+    if (auto *VD = dyn_cast<VarDecl>(D)) {
+      if (FILEType && !ACtx.hasSameType(*FILEType, VD->getType()))
+        continue;
----------------
It will early return and uses one fewer `continue`.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:504
+
+  OrigStdin = findStdStream("stdin", C);
+  OrigStdout = findStdStream("stdout", C);
----------------
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.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:554
 
+  auto AssumeRetValNotEq = [&C, &StateNotNull, RetVal](SymbolRef StdStream) {
+    SVal NotEqS = C.getSValBuilder().evalBinOp(
----------------
It might be a personal preference but I think lambdas should be pure in the sense that it takes something and produces something else.
Regardless of your choice, the trailing return type would highlight it.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:559-562
+    Optional<DefinedOrUnknownSVal> NotEqDef =
+        NotEqS.getAs<DefinedOrUnknownSVal>();
+    if (!NotEqDef)
+      return;
----------------
I think you should be fine with `castAs()`. I'm not expecting this to fail.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:555
+        C.getSValBuilder().makeSymbolVal(StdStream),
+        C.getASTContext().getLogicalOperationType());
+    Optional<DefinedOrUnknownSVal> NotEqDef =
----------------
`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.


================
Comment at: clang/test/Analysis/stream.c:276-277
+    return;
+  if (F != stdin && F != stdout && F != stderr)
+    fclose(F); // no warning: the opened file can not be equal to std streams
+}
----------------
How about checking this way instead?
```lang=C++
clang_analyzer_eval(F != stdin);  // TRUE
clang_analyzer_eval(F != stdout); // TRUE
clang_analyzer_eval(F != stderr); // TRUE
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106644/new/

https://reviews.llvm.org/D106644