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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
