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