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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
