martong added a comment.
In D77658#2065174 <https://reviews.llvm.org/D77658#2065174>, @MaskRay wrote:
> `arc` adds many unneeded tags from Phabricator. You can drop `Reviewers:`
> `Subscribers:` `Tags:` and the text `Summary:` with the following script:
>
> arcfilter () {
> arc amend
> git log -1 --pretty=%B | awk '/Reviewers:|Subscribers:/{p=1} /Reviewed
> By:|Differential Revision:/{p=0} !p && !/^Summary:$/ {sub(/^Summary:
> /,"");print}' | git commit --amend --date=now -F -
> }
>
>
> `Reviewed By: ` is considered important by some people
> (https://lists.llvm.org/pipermail/llvm-dev/2020-January/137889.html). You
> should keep the tag. (I started to use `--date=now` because some people find
> author date != committer date annoying. The committer date is usually what
> people care.))
Thanks for pointing this out and for the script! I'll try to omit unnecessary
tags from arc in the future.
================
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:
> > 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.
Alright, I renamed `matches` to `matchesAndSet` and we set the FD in this
function now. Also `setFunctionDecl` is removed.
================
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;
----------------
balazske wrote:
> 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").
Ok, I removed this outdated comment.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:116
+
+ /// Do sanity check on the constraint.
+ bool checkValidity(const FunctionDecl *FD) const {
----------------
balazske wrote:
> 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.
We check here, whether the constraint is malformed or not. It is malformed if
the specified argument has a mismatch with the given FunctionDecl (e.g. the arg
number is out-of-range of the function's arguments list).
Added a comment in the code about that.
================
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;
----------------
balazske wrote:
> 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.
Indeed, I agree. Removed `auto` and refreshed the outdated variable names.
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
