[cfe-dev] New Matchers

Gábor Horváth xazax.hun at gmail.com
Mon Sep 17 04:25:32 PDT 2012


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

On 17 September 2012 08:38, Daniel Jasper <djasper at google.com> wrote:

> 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/ce98622e/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: C++11Matchers.diff
Type: application/octet-stream
Size: 4775 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20120917/ce98622e/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: cStyleCast.diff
Type: application/octet-stream
Size: 1794 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20120917/ce98622e/attachment-0001.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: trivialMatchers.diff
Type: application/octet-stream
Size: 6110 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20120917/ce98622e/attachment-0002.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: unsupported.diff
Type: application/octet-stream
Size: 4598 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20120917/ce98622e/attachment-0003.obj>


More information about the cfe-dev mailing list