This is an archive of the discontinued LLVM Phabricator instance.

Add matchers for gtest's ASSERT_THAT, EXPECT_THAT, ON_CALL and EXPECT_CALL
ClosedPublic

Authored by zhaomo on May 26 2021, 11:49 AM.

Details

Summary

This patch adds support for matching gtest's ASSERT_THAT, EXPECT_THAT, ON_CALL and EXPECT_CALL macros.

Diff Detail

Event Timeline

zhaomo created this revision.May 26 2021, 11:49 AM
zhaomo requested review of this revision.May 26 2021, 11:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 26 2021, 11:49 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
zhaomo updated this revision to Diff 348093.May 26 2021, 2:12 PM
zhaomo updated this revision to Diff 348094.May 26 2021, 2:15 PM
zhaomo updated this revision to Diff 348095.
zhaomo updated this revision to Diff 348113.May 26 2021, 3:26 PM
hokein added inline comments.May 26 2021, 11:52 PM
clang/include/clang/ASTMatchers/GtestMatchers.h
34

worth comments.

out of curiosity, what do we call this Matchers? I'd be careful to introduce a different matcher concept in the ast_matcher library, it would easily cause confusion.

34–44

as we add a new method to handle ASSERT_THAT, this comment is not clear enough to me, ASSERT_... makes me think ASSERT_THAT is included as well.

I would suggest rephrase the comment (explicitly mentioning this is for comparison operations, IIUC), and even rename the method to gtestAssertCmp (we can defer it to a follow-up patch).

45

nit: I would reorder APIs in these file, grouping by assert, expect, oncall.

...gtestAssert();
...gtestAssertThat();

...gtestExpect();
...gtestExpectThat();
...gtestExpectCall();
91

this comment doesn't seem to express enough information, what's the difference from the above one? I think adding an example would be helpful.

clang/lib/ASTMatchers/GtestMatchers.cpp
41

why remove this llvm_unreachable? I think this is a common practice in LLVM.

49–56

the static qualifier is not needed as you wrap it within an anonymous namespace. the same below.

88

worth comments on these magic string come from -- gtest library?

110

As the function creates an AST matcher to match the gtest method, 'd rename the function name like gtestComparisonIMatcher, the same to following functions.

141

I believe this is something related to the gtest internal implementation, could you add a comment to explain that?

clang/unittests/ASTMatchers/GtestMatchersTest.cpp
330

nit: bind is not needed?

ymandel added inline comments.May 27 2021, 5:05 AM
clang/include/clang/ASTMatchers/GtestMatchers.h
34

+1 Please move the comments relating to this enum from the two gtestOnCall functions to here.

hokein@ -- the "Matchers" is because its describing googletest matchers. But, it is a bit confusing in this context and doesn't really add anything. What do you think of None and Some instead?

65–69

Please move this to a comment describing the MockArgs enum.

clang/lib/ASTMatchers/GtestMatchers.cpp
49–56

nit: per the style guide (https://releases.llvm.org/2.7/docs/CodingStandards.html#micro_anonns), I think it would be better to shrink the anonymous namespace to only enclose the enum decl, and keep these static annotations and in fact add a few that are currently missing on the gtestCallInternal functions.

hokein added inline comments.May 27 2021, 6:28 AM
clang/include/clang/ASTMatchers/GtestMatchers.h
34

What do you think of None and Some instead?

this sounds good to me.

clang/lib/ASTMatchers/GtestMatchers.cpp
49–56

Up to you -- I'm fine with either (these two styles exit in LLVM codebase)- using anonymous namespace aligns more with google style...

zhaomo updated this revision to Diff 348384.May 27 2021, 2:01 PM
zhaomo marked 4 inline comments as done.
zhaomo added inline comments.
clang/include/clang/ASTMatchers/GtestMatchers.h
34–44

I changed the comment to make it more accurate.

ymandel@ and I talked about rename the APIs but we weren't sure if it is worth it as it may break some code. I would like to at least defer that to another patch.

91

The difference between the two gtestExpectCall overloads is just like the difference between the two gtestOnCall overloads.

clang/lib/ASTMatchers/GtestMatchers.cpp
41

ymandel@ suggested me removing it as the switch covers all the possible values of the enum.

49–56

I changed the code following ymandel@'s suggestion.

clang/unittests/ASTMatchers/GtestMatchersTest.cpp
330

Just wanted to illustrate the usage here.

zhaomo updated this revision to Diff 348399.May 27 2021, 3:11 PM

a few more nits, the code looks good to me now. As discussed with @ymandel offline, we should be aware of that before moving forward to this direction -- this patch will likely have the gtest-versioning issue.

clang/lib/ASTMatchers/GtestMatchers.cpp
41

either seems fine to me (this is just a coding style), I think the problem is that we have inconsistencies in the patch - e.g. on Line95, default: llvm_unreachable, we should stick with one style.

110

this comment seems undone.

134

I think For example, ... words are implementation details, should move to the corresponding switch case below.

zhaomo updated this revision to Diff 348998.Jun 1 2021, 9:52 AM
zhaomo marked an inline comment as done.
zhaomo added inline comments.
clang/lib/ASTMatchers/GtestMatchers.cpp
41

The switch statement there does not cover all the cases so llvm_unreachable is necessary.

110

Changed it a bit and moved it to the top of this file.

zhaomo updated this revision to Diff 349030.Jun 1 2021, 11:13 AM
hokein accepted this revision.Jun 2 2021, 5:48 AM
This revision is now accepted and ready to land.Jun 2 2021, 5:48 AM
ymandel accepted this revision.Jun 2 2021, 6:17 AM

Nice!

zhaomo added a comment.Jun 2 2021, 9:43 AM

Thanks folks!