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

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

balazske added a comment.

It is better now, but the refactoring change should be in a separate revision (it affects the other already added functions too).



================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:350
+      } else {
+        *this = Signature(Args, *RetTy);
       }
----------------
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.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:378
 
   static QualType getArgType(const FunctionDecl *FD, ArgNo ArgN) {
     assert(FD && "Function must be set");
----------------
Check for isInvalid here (with assert)?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:882
+  const QualType SizePtrTy = getPointerTy(SizeTy);
+  const QualType SizePtrRestrictTy = getRestrictTy(SizePtrTy);
 
----------------
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()`, ... .
```


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1026
   Optional<QualType> FilePtrTy, FilePtrRestrictTy;
   if (FileTy) {
     // FILE *
----------------
These `if`s can be removed too (in another patch).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84415