martong added a comment.
Thanks @balazske for your comments, you always make an assiduous review!
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:350
+ } else {
+ *this = Signature(Args, *RetTy);
}
----------------
balazske wrote:
> Not very bad but I do not like to call another constructor and assignment
> here, this can be many redundant operations. It is better to fill the
> internal arrays here and have a separate assert function that checks for
> validness (of the internal arrays) instead of the private constructor.
Ok, makes sense, thanks!
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:378
static QualType getArgType(const FunctionDecl *FD, ArgNo ArgN) {
assert(FD && "Function must be set");
----------------
balazske wrote:
> Check for isInvalid here (with assert)?
We cannot do that, this is not a member function of the Signature, this
operates on a FuncitonDecl. But it makes sense to add the assert to the
`mathces` member function.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:882
+ const QualType SizePtrTy = getPointerTy(SizeTy);
+ const QualType SizePtrRestrictTy = getRestrictTy(SizePtrTy);
----------------
balazske wrote:
> Another idea: Have a class that handles all the variants (simple, pointer,
> const pointer, const restrict pointer, restrict). It can have get functions
> that compute the type if not done yet, or get every variant at first time
> even if it is later not used (or has no sense).
> For example create it like `TypeVariants SizeTy{ACtx.getSizeType()};` and
> then call `SizeTy.getType()`, `SizeTy.asPtr()`, `SizeTy.asPtrRestrict()`, ...
> .
> ```
I'd rather keep the current form, because I think it makes it easier to compose
types from different base types (and it is similar to the ASTMatchers in this
sense).
================
Comment at:
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1026
Optional<QualType> FilePtrTy, FilePtrRestrictTy;
if (FileTy) {
// FILE *
----------------
balazske wrote:
> These `if`s can be removed too (in another patch).
Yep, I am going to create another base patch.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D84415/new/
https://reviews.llvm.org/D84415
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
