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

[Balázs Kéri via Phabricator via cfe-commits]

balazske marked 2 inline comments as done.
balazske 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;
+
----------------
Szelethus wrote:
> 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...
Should be the "stream functions" always in system header? If no `isInSystemHeader` call is used any global function called "fopen" with 2 arguments is recognized as stream opening function. If it returns a pointer and no "fclose" is called on that we will get a resource leak warning for that code. (But for this case the user will probably not enable the checker?).


================
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:
> 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?
This can not be an assert because it is possible to have global functions with same name but different signature. The check can be kept to filter out such cases. The wanted stream functions have only integral or enum or pointer arguments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67706