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