[PATCH] D84415: [analyzer][StdLibraryFunctionsChecker] Add POSIX pthread handling functions

[Gabor Marton via Phabricator via cfe-commits]

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