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

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to