NoQ added inline comments.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:60-61
private:
- void Fopen(CheckerContext &C, const CallExpr *CE) const;
- void Tmpfile(CheckerContext &C, const CallExpr *CE) const;
- void Fclose(CheckerContext &C, const CallExpr *CE) const;
- void Fread(CheckerContext &C, const CallExpr *CE) const;
- void Fwrite(CheckerContext &C, const CallExpr *CE) const;
- void Fseek(CheckerContext &C, const CallExpr *CE) const;
- void Ftell(CheckerContext &C, const CallExpr *CE) const;
- void Rewind(CheckerContext &C, const CallExpr *CE) const;
- void Fgetpos(CheckerContext &C, const CallExpr *CE) const;
- void Fsetpos(CheckerContext &C, const CallExpr *CE) const;
- void Clearerr(CheckerContext &C, const CallExpr *CE) const;
- void Feof(CheckerContext &C, const CallExpr *CE) const;
- void Ferror(CheckerContext &C, const CallExpr *CE) const;
- void Fileno(CheckerContext &C, const CallExpr *CE) const;
-
- void OpenFileAux(CheckerContext &C, const CallExpr *CE) const;
-
- ProgramStateRef CheckNullStream(SVal SV, ProgramStateRef state,
- CheckerContext &C) const;
- ProgramStateRef CheckDoubleClose(const CallExpr *CE, ProgramStateRef state,
- CheckerContext &C) const;
+ typedef void (StreamChecker::*FnCheck)(const CallEvent &,
+ CheckerContext &) const;
+
----------------
Szelethus wrote:
> Prefer using. When I wrote D68165, I spent about 10 minutes figuring out how
> to do it... Ah, the joys of the C++ syntax.
> ```lang=c++
> using FnCheck = void (StreamChecker::*)(const CallEvent &,
> CheckerContext &) const;
> ```
It's actually very easy to remember, it's just an alien kiss smiley ::*)
================
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;
+ }
----------------
balazske wrote:
> 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.
Clang shouldn't crash even when the library definition is incorrect.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:156
+void StreamChecker::evalFread(const CallEvent &Call, CheckerContext &C) const {
+ (void)CheckNullStream(Call.getArgSVal(3), C);
}
----------------
Szelethus wrote:
> 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
I'd rather not discard the return value, but allow callbacks indicate an error
when something goes wrong, so that to allow them to abort `evalCall()`.
But more importantly, note how `CheckNullStream` actually returns a
`ProgramStateRef` that was meant to be transitioned into. Which means that the
primary execution path actually gets cut off.
So even if buildbots didn't warn on this, i wish they did.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D67706/new/
https://reviews.llvm.org/D67706
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
