Page MenuHomePhabricator

[ASTMatchers][NFC] Add tests for assorted `CXXMemberCallExpr` matchers.
ClosedPublic

Authored by ymandel on Jan 17 2019, 5:47 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

ymandel created this revision.Jan 17 2019, 5:47 AM
ymandel retitled this revision from Add tests for assorted `CXXMemberCallExpr` matchers. to [ASTMatchers] Add tests for assorted `CXXMemberCallExpr` matchers..Jan 17 2019, 5:51 AM

Please always subscribe lists.
The reviews that happen without lists are null and void.

Is this testing pre-existing behavior, and thus NFC?

lebedev.ri set the repository for this revision to rC Clang.Jan 17 2019, 5:57 AM

Please always subscribe lists.
The reviews that happen without lists are null and void.

Thanks for updating my diffs with the relevant list. Noted for next time.

Is this testing pre-existing behavior, and thus NFC?

Yes.

ymandel retitled this revision from [ASTMatchers] Add tests for assorted `CXXMemberCallExpr` matchers. to [ASTMatchers][NFC] Add tests for assorted `CXXMemberCallExpr` matchers..Jan 17 2019, 6:14 AM
steveire added inline comments.Jan 17 2019, 6:28 AM
clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
475 ↗(On Diff #182262)

Could you add a test which uses the same c++ snippet as this, but with a matcher using hasName("Y")?

ymandel updated this revision to Diff 182295.Jan 17 2019, 8:12 AM

Extended test for matcher on.

ymandel marked 2 inline comments as done.Jan 17 2019, 8:18 AM
ymandel added inline comments.
clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
475 ↗(On Diff #182262)

Good suggestion -- this caught a mistake in the comments. I'll update the other diff to reflect.

Thanks, do you need someone to commit this for you?

clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
492 ↗(On Diff #182295)

In the OnImplicitObjectArgument test, you don't check snippet 1 because it has no X. This line is probably not needed.

558 ↗(On Diff #182295)

Are we missing a matcher that would reach the type of X in this case? hasImplicitObjectExpression, or something equivalent?

ymandel updated this revision to Diff 182919.Jan 22 2019, 7:57 AM
ymandel marked an inline comment as done.

Remove unnecessary expectation.

ymandel updated this revision to Diff 182933.Jan 22 2019, 8:46 AM
ymandel marked 3 inline comments as done.

Extended test of hasObjectExpression.

ymandel added inline comments.Jan 22 2019, 8:48 AM
clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
558 ↗(On Diff #182295)

Good question. The reason we're not reaching X in this case is that we're distinguishing between T and T* (like thisPointerType does). That's separate from the difference between on and onImplicitObjectArgument. So, I think we'll at least want a matcher that elides this difference, e.g.
hasObjectType.

I'm less inclined to add matchers that distinguish the written from the coerced member expression (one variant each for the expression and type matchers) given that I think this issue comes up far less for member expressions that don't involve calls. I'm just afraid that the proliferation of matchers might confuse beginners.

Note that I extended the test to include the hasPointerType() case (especially since it's now mentioned in the comments).

Aaron, Alex, any additional comments on this change?

This revision is now accepted and ready to land.Feb 5 2019, 10:55 AM
steveire accepted this revision.Feb 5 2019, 1:12 PM

Sorry, this fell off my radar :). LGTM too.

ymandel updated this revision to Diff 187003.Feb 15 2019, 6:08 AM

Sync to head.

Herald added a project: Restricted Project. · View Herald TranscriptFeb 15 2019, 6:08 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 15 2019, 6:42 AM

This broke compilation with GCC 5.4 on Ubuntu 16.04:

../tools/clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp:474:243: warning: missing terminating " character
../tools/clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp:474:3: error: missing terminating " character
   EXPECT_TRUE(matches(
   ^
../tools/clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp:480:7: error: stray ‘\’ in program
         void z(X x) { x.m(); }
       ^
ormris added a subscriber: ormris.Feb 15 2019, 2:30 PM