[cfe-commits] New Matchers

[Daniel Jasper]

+ 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 at 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


On Tue, Sep 18, 2012 at 3:26 PM, Daniel Jasper <djasper at google.com> wrote:

>
>
>
> 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-commits/attachments/20120919/873c3939/attachment.html>