Page MenuHomePhabricator

[analyzer] LoopUnrolling: update the matched assignment operators
ClosedPublic

Authored by szepet on Oct 14 2017, 5:50 AM.

Diff Detail

Repository
rC Clang

Event Timeline

szepet created this revision.Oct 14 2017, 5:50 AM
szepet edited reviewers, added: zaks.anna; removed: anna.Oct 14 2017, 5:51 AM

LGTM.. however I would like approval from somebody else also.

NoQ edited edge metadata.Oct 16 2017, 9:02 AM

Neat! Tests?

xazax.hun added inline comments.Oct 19 2017, 2:26 AM
lib/StaticAnalyzer/Core/LoopUnrolling.cpp
99–100

Maybe instead of enumerating all the assignment operators here it would be better to have a matcher that checks isAssignmentOp for CXXOperatorCalls/BinaryOperators? This would be less error prone and more future proof.

szepet updated this revision to Diff 120732.EditedOct 28 2017, 4:39 AM
  • Updated to use a custom AST matcher for assignment operator check. (More structured and efficient.)
  • Tests added as well.
szepet marked an inline comment as done.Oct 28 2017, 4:40 AM
NoQ added a comment.EditedOct 30 2017, 4:43 AM

Yay.

Thanks Gabor, i didn't think about it but it looks very nice to have such matcher.

I think the matcher shouldn't be checker-local, but shared into ASTMatchers.h all other matchers - not only this matcher is universally useful, but also i have weird vague memories that matchers may collide by name if defined in different translation units differently (if this is indeed this way then having the same matcher in multiple places is very bad).

xazax.hun edited edge metadata.Nov 13 2017, 3:10 AM

I agree it might be useful to expose this matcher to everybody.

szepet updated this revision to Diff 123176.Nov 16 2017, 6:55 AM
szepet added a reviewer: klimek.

isAssignmentOp matcher moved to ASTMatchers.h
(Manuel added to reviewers as the code owner of ASTMatchers)

Not sure how clang/docs/LibASTMatchersReference.html is supposed to be updated.

include/clang/ASTMatchers/ASTMatchers.h
3931 ↗(On Diff #123176)

Maybe

-/// \brief Matches all kind of assignment operators.
+/// \brief Matches all kinds of assignment operators.
3938 ↗(On Diff #123176)

Needs unittests, probably in clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp ?

szepet updated this revision to Diff 123187.Nov 16 2017, 8:13 AM
szepet marked 2 inline comments as done.

Testfiles added and HTML updated.

xazax.hun accepted this revision.Nov 17 2017, 9:31 AM

A nit, otherwise LG!

lib/StaticAnalyzer/Core/PathDiagnostic.cpp
693 ↗(On Diff #123187)

Is this change related? Maybe you forgot to add a dependency to this revision?

This revision is now accepted and ready to land.Nov 17 2017, 9:31 AM

Added Alexander and Aaron as reviewers for the matcher parts.

aaron.ballman edited edge metadata.Dec 1 2017, 5:31 AM

Aside from a request for another test, the matcher parts LGTM.

unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
1992 ↗(On Diff #123187)

Can you also add a test like:

struct S { S& operator=(const S&); };
void x() { S s1, s2; s1 = s2; }
alexfh removed a reviewer: alexfh.Jan 31 2018, 3:48 AM
alexfh added a subscriber: alexfh.Mar 14 2018, 8:35 AM

Do you want to submit the matcher part separately?

szepet marked 2 inline comments as done.

Do you want to submit the matcher part separately?

Yepp, but added as a different patch since implemented it for cxxOperatorCall as well. (I guess Aaron's test request implied it and I find it useful as well.)
See: https://reviews.llvm.org/D44893

szepet updated this revision to Diff 139776.Mar 26 2018, 5:31 AM

Added the assignment operator matcher patch as a dependency and rebased on top of that.

This revision was automatically updated to reflect the committed changes.