[cfe-dev] New Matchers

[Daniel Jasper]

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));
  +

If you break function call arguments onto multiple lines, align the
subsequent lines with the first argument (here 'lambdaExpr' right under
'"auto..'). Also at all other places in your patch.

  +TEST(CStyleCast, MatchesSimpleCase) {
  +  EXPECT_TRUE(matches("char* p = (char*)(&p);",
  +                         expr(cStyleCastExpr())));
  +}

This does not seem like a very simple case to me ;-). How about something
like "int i = (int) 2.2f;"? I don't think you need the 'expr()' around the
'cStyleCastExpr()' (also at most other places).

  +TEST(CStyleCast, DoesNotMatchOtherCasts) {
  +  EXPECT_TRUE(notMatches("char* p = static_cast<char*>(0);",
  +                         expr(cStyleCastExpr())));
  +  ..

Putting all the cases into one code snippet and the have cStyleCastExpr()
not match on it might be slightly easier to read.

I'll take another look after you have split up the patches, but I think we
can get most of them in quite quickly. Note that we are currently testing a
code review system on http://llvm-reviews.chandlerc.com/. I personally find
it easier to review patches there, so feel free to use it (but it is of
course entirely optional).

Cheers,
Daniel


On Fri, Sep 14, 2012 at 10:16 PM, Gábor Horváth <xazax.hun at gmail.com> wrote:

> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20120917/890a82a2/attachment.html>