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

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1006
+        return IntRangeVector{std::pair<RangeInt, RangeInt>{b, *e}};
+      return IntRangeVector{};
+    }
----------------
balazske wrote:
> This return of empty vector and possibility of adding empty vector to range constraint is a new thing, probably it is better to check at create of range constraint (or other place) for empty range (in this case the summary could be made invalid)? But this occurs probably never because the matching type (of the max value) should be used at the same function and if the type is there the max value should be too.
Alright, I added an assert to RangeConstraint::getRanges:
```
     const IntRangeVector &getRanges() const {
+      // When using max values for a type, the type normally should be part of
+      // the signature. Thus we must had looked up previously the type. If the
+      // type is not found then the range would be empty, but then the summary
+      // should be invalid too.
+      assert(Args.size() && "Empty range is meaningless");
       return Args;
     }
```


================
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:
> martong wrote:
> > 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.
> I still like it better if all the type variables are created at one place (can be more easy to maintain if order changes and we have one block of types and one of functions) but this is no reason to block this change.
I see your point, still I'd keep this way, because I this way the functions and the types they use are close to each other in the source.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86531