[cfe-dev] New Matchers

[Daniel Jasper]

On Mon, Sep 17, 2012 at 1:25 PM, Gábor Horváth <xazax.hun at gmail.com> wrote:

> 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


>
>
> 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/20120918/4862a515/attachment.html>