[PATCH] D67706: [clang][analyzer] Using CallDescription in StreamChecker.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:109-110
 bool StreamChecker::evalCall(const CallEvent &Call, CheckerContext &C) const {
+  if (!Call.isGlobalCFunction())
+    return false;
+
----------------
balazske wrote:
> Szelethus wrote:
> > Isn't this redundant with my other inline about parameter types?
> Probably change to `isInSystemHeader` or use both?
Actually, this looks fine. How about preserving this...


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:127-131
+  for (auto P : Call.parameters()) {
+    QualType T = P->getType();
+    if (!T->isIntegralOrEnumerationType() && !T->isPointerType())
+      return nullptr;
+  }
----------------
Szelethus wrote:
> balazske wrote:
> > Szelethus wrote:
> > > I'm not sure why we need this, is it true that *all* stream related functions return a pointer or a numerical value? Are we actually checking whether this really is a library function? If so, this looks pretty arbitrary.
> > This comes from code of CStringChecker:
> > ```
> >   // Pro-actively check that argument types are safe to do arithmetic upon.
> >   // We do not want to crash if someone accidentally passes a structure
> >   // into, say, a C++ overload of any of these functions. We could not check
> >   // that for std::copy because they may have arguments of other types.
> > ```
> > Still I am not sure that the checker works correct with code that contains similar named but "arbitrary" functions.
> Oops, meant to write that ", is it true that *all* stream related functions have only pointer or a numerical parameters?".
...and removing this one, or changing it to an assert?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:156
+void StreamChecker::evalFread(const CallEvent &Call, CheckerContext &C) const {
+  (void)CheckNullStream(Call.getArgSVal(3), C);
 }
----------------
balazske wrote:
> Szelethus wrote:
> > Why the need for `(void)`? `CheckNullSteam` doesn't seem to have an `LLVM_NODISCARD` attribute.
> I wanted to be sure to get no buildbot compile errors (where -Werror is used).
They actually break on this?? Let me guess, is it the windows one? :D


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67706