[cfe-commits] New Matchers

Daniel Jasper djasper at google.com
Tue Sep 18 23:05:11 PDT 2012

+ 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:
submitted as r164123.

  +/// \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.

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.

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.


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>

More information about the cfe-commits mailing list