[PATCH] D77658: [analyzer] StdLibraryFunctionsChecker: Add sanity checks for constraints

[Gabor Marton via Phabricator via cfe-commits]

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


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:697
         if (auto *FD = dyn_cast<FunctionDecl>(D)) {
-          if (S.matchesSignature(FD)) {
+          if (S.Sign.matches(FD) && S.validate(FD)) {
             auto Res = Map.insert({FD->getCanonicalDecl(), S});
----------------
balazske wrote:
> martong wrote:
> > Szelethus wrote:
> > > This looks a bit odd, we're checking whether the function matches, and than we validate right after? Shouldn't we just not match the `FD` if it isn't valid?
> > Yeah, ok, I moved the validation back into `matchesSignature`.
> I think these are not related, there should be signature match and validness check, but it is natural to check first the signature, then the validness.
@Szelethus, @balazske : I agree with both of you so I renamed `matchesSignature` to `matches`, which is a shorthand to the the signature match and then the validity check (and added a comment):
```
    // Returns true if the summary should be applied to the given function.
    bool matches(const FunctionDecl *FD) const {
      return Sign.matches(FD) && validateByConstraints(FD);
    }
```


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:131
+    /// Do polymorphic sanity check on the constraint.
+    virtual bool sanityCheck(const FunctionDecl *FD) const { return true; }
   };
----------------
balazske wrote:
> Function names should be verb phrases. Maybe `checkSanity` is good or `validateInternal`, `validateMore` (this function relates to `validate`, not something different from it as "sanity").
> 
> `validate` can be called `isValid` or `checkValidity`, so we can not think (from the name) that the function changes something on the object. (`checkValidity` is good, then `sanityCheck` can be renamed to `checkSpecificValidity` or like this.)
Thanks for the naming suggestions, they are definitely better than my naming!


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:304
   ///     If these constraints are not satisfied that means a fatal error
   ///     usually resulting in undefined behaviour.
+  class Summary {
----------------
balazske wrote:
> I would extend the documentation with information about that the signature and constraints together contain information about what functions are accepted by the summary. The signature can use "wildcards" (the irrelevant types) and the constraints may specify additional rules for that type (that comes from the kind of constraint).
OK, I added some more docs to the class.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:345
+    // the given constraints.
+    bool validate(const FunctionDecl *FD) const {
+      for (const auto &Case : CaseConstraints)
----------------
balazske wrote:
> Again `isValid` or `checkValidity` is a better name, or `validateByConstraints`, `matchesConstraints`. Additionally `matchesSignature` checks in its current form not only for signature, so its name is not correct too.
Ok, renamed to `validateByConstraints`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77658