New Matchers

Greetings!

I am interested in participating in developing the tooling library. To get started I tried to reach the low hanging fruits, and added some matchers that were missing, I hope at least some of them will be pushed to upstream. I also included a some C++11 node matchers.

The diff is attached. Any feedback is welcome.

Thanks,
Gabor

matchers.diff (11.3 KB)

Hi Gábor,

thanks for working on this. In general, although everything in your patch are “matchers”, I’d prefer if you can make multiple smaller patches out of it. In this cases, I would like to have 4 patches:

  1. cStyleCastExpr (already discussed, no-brainer)
  2. C++11-specific matchers (so we can decide whether the patch includes everything to support them)
  3. Matchers where something seems off (“not yet supported?”)
  4. The remaining trivial matchers (breakStmt, continueStmt, …)

As for the patch itself:

testing::AssertionResult matchesConditionally(const std::string &Code,
const T &AMatcher,

  • bool ExpectMatch) {
  • bool ExpectMatch,
  • bool CXX11 = false) {

The indent is off. Also, add the default value for the new parameter to the two existing call sites and not as a parameter default. There is just one test case that fails with -std=c++11, so we should probably make that the default language for matches() and notMatches().

Also, I find methods with multiple bool parameters confusing as I can never remember which one means which. As it is somewhat likely that we will want to support more language variants, I would turn this parameter either into an enum or into a string.

+TEST(Matcher, Lambda) {

  • EXPECT_TRUE(matchesConditionally(“auto f = [&] (int i) { return i; };”,
  • lambdaExpr(), true, true));

Hi!

Thanks for your review. I readjusted my diff, and sliced it to 4 parts (however I did it manually, so there may be problems when one wants to apply them automatically, if there is a better way to slice diffs, please let me know).

The expr()s are necessary, because those matchers are implemented via VariadicDynCastAllOfMatcher, where the SourceT is Expr. I implemented it that way, because I mimiced the existing implementations, for example the reinterpret_cast matcher (probably it was implemented that way to increase type safety?).

I will explore http://llvm-reviews.chandlerc.com/ later, thanks for letting me know.

Gábor

C++11Matchers.diff (4.66 KB)

cStyleCast.diff (1.75 KB)

trivialMatchers.diff (5.97 KB)

unsupported.diff (4.49 KB)

Hi!

Thanks for your review. I readjusted my diff, and sliced it to 4 parts
(however I did it manually, so there may be problems when one wants to
apply them automatically, if there is a better way to slice diffs, please
let me know).

As discussed on IRC, I think using git (git-svn) is a very good option to
juggle multiple LLVM/Clang changes.

The expr()s are necessary, because those matchers are implemented via

VariadicDynCastAllOfMatcher, where the SourceT is Expr. I implemented it
that way, because I mimiced the existing implementations, for example the
reinterpret_cast matcher (probably it was implemented that way to increase
type safety?).

I have just changed that everywhere else. It was there for historic reasons
and does not really provide additional type-safety.

I will explore http://llvm-reviews.chandlerc.com/ later, thanks for letting

me know.

Gábor

Cheers,
Daniel

  • cfe-commits

In general, if you send further versions of the patches, please put each into a separate mail with a more specific subject. That way, it is easier for others on the list to follow. Also, as these patches are close to being submitted, include the list cfe-commits@cs.uiuc.edu.

As for the patches themselves:
cStyleCast.diff:
submitted as r164123.

trivialMatchers.diff:

+/// \endcode
+/// attributedStmt()
+/// matches ‘__asm(“mov al, 2”)’
+const internal::VariadicDynCastAllOfMatcher<Stmt, AsmStmt> asmStmt;

This should bei asmStmt() in the comment. Also the patch does not apply (“malformed patch …”), please try to create one that applies cleanly.

C++11Matchers.diff:
As already stated in my previous email, lets make “-std=c++11” the default (i.e. set this by matches() and notMatches()). This will make one of the current tests break, so change that to use matchesConditionally with “-std=gnu++98”. Also, the patch is malformed.

unsupported.diff:
Do you need any of those for your current project? If yes, I can take a look at what does not work and what is missing, but I rather not do that, just to have a more complete matcher interface right now.

Cheers,
Daniel

Hi!

Once I switched to git (probably today or tomorrow), I will generate new diff files, and submit them to phabricator (and send a mail to the list with a subject specific to the patch).

Fortunately I do not need any matchers from the unsupported diff, it was just some kind of experiment if they would work yet, so we should not bother with them at all.

Thanks,
Gábor