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

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

balazske 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});
----------------
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.


================
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; }
   };
----------------
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.)


================
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 {
----------------
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).


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:345
+    // the given constraints.
+    bool validate(const FunctionDecl *FD) const {
+      for (const auto &Case : CaseConstraints)
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77658