[cfe-dev] New Matchers

Wed Sep 19 03:19:10 PDT 2012

Hi!

Once I switched to git (probably today or tomorrow), I will generate new
diff files, and submit them to phabricator (and send a mail to the list
with a subject specific to the patch).

Fortunately I do not need any matchers from the unsupported diff, it was
just some kind of experiment if they would work yet, so we should not
bother with them at all.

Thanks,
Gábor

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

> + 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-dev/attachments/20120919/50dc6675/attachment.html>