[PATCH] D86531: [analyzer][StdLibraryFunctionsChecker][NFC] Use Optionals throughout the summary API

[Gabor Marton via Phabricator via cfe-commits]

martong marked 3 inline comments as done.
martong added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:851
+  } getPointerTy(ACtx);
+  class {
+  public:
----------------
balazske wrote:
> Why has this class different layout than `GetPointerTy` and `GetRestrictTy` (beneath that here is no `ASTContext`)? It would be better if all these classes look the same way: First is the operator with `QualType`, then operator with `Optional` that calls the other version of the operator. And all of these can be "unnamed" classes?
I'd had the same thoughts before, and was thinking about that maybe a class template would suffice for all these, but then I realized the following:

To make a type const, we don't need the ASTContext, that is the reason of the difference.
However, to make a type restricted or a pointer, we need the ASTContext. This is a legacy non-symmetry from the AST.
> And all of these can be "unnamed" classes?
GetPointerTy and GetRestrictTy need the ASTContext in their constructor. And you cannot define a constructor to an unnamed class because you cannot write down the nonexistent name.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:890
 
-  const QualType VoidPtrTy = ACtx.VoidPtrTy; // void *
-  const QualType IntPtrTy = ACtx.getPointerType(IntTy); // int *
+  const QualType VoidPtrTy = getPointerTy(VoidTy); // void *
+  const QualType IntPtrTy = getPointerTy(IntTy);   // int *
----------------
balazske wrote:
> Is it better to use `ACtx.VoidPtrTy`? 
I'd like to use the `ACtx` as less as possible: just to get the basic types. And from that on we can use our convenience API (GetConstTy, GetPointerTy, etc) to define all of the derived types... and we can do that in a declarative form (like ASTMatchters).
Also, the declarative definition of these types now look the same regardless the type is "derived" from a looked-up type or from a built-in type.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1321
 
-    Optional<QualType> Mode_tTy = lookupType("mode_t", ACtx);
+    Optional<QualType> Mode_tTy = lookupTy("mode_t");
 
----------------
balazske wrote:
> It is better to get every type at the start before adding functions, or group the functions and get some types at the start of these groups but mark the groups at least with comments.
Well, with looked-up types I followed the usual convention to define a variable right before using it. This means that we lookup a type just before we try to add the function which first uses that type.

However, builtin types are defined at the beginning, because they are used very often.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86531