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

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

balazske added a comment.

The code looks basically good to me, some documentations can be improved.



================
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:
> 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);
>     }
> ```
Suggestion: Maybe we can have one function for `matches` and `setFunctionDecl` (set it if matches). We do not want to change the function decl later anyway, and not to something that does not match.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:68
   /// We avoid nesting types here because each additional qualifier
   /// would need to be repeated in every function spec.
+  class Summary;
----------------
This text above is not the documentation of `Summary` (is it attached to `Summary` by doxygen?). Probably not `///` is needed, only `//`. And it is probably out of date now, at least I can not understand immediately what is it about (what typedef's are these, what kind of "nesting types"). 


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:116
+
+    /// Do sanity check on the constraint.
+    bool checkValidity(const FunctionDecl *FD) const {
----------------
We check here that a function is a suitable candidate to be used with the constraint? (We select a function for the constraint, not a constraint for a function.) Maybe a better description would help to clarify this.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:571
   // Apply case/branch specifications.
-  for (const auto &VRS : Summary.CaseConstraints) {
+  for (const auto &VRS : Summary.getCaseConstraints()) {
     ProgramStateRef NewState = State;
----------------
It may be better to see type of `VRS` instead of `auto` (and know what the `VRS` abbrevation means, why not `CC` for case constraint and not `VC` for `ValueConstraint`). Same for `VR` below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77658