[cfe-dev] Patch: AST Matcher Framwork and an example tool

[Manuel Klimek]

On Fri, Jun 17, 2011 at 10:22 AM, Chris Lattner <clattner at apple.com> wrote:

> On Jun 14, 2011, at 11:21 PM, Manuel Klimek wrote:
>
> Considering that we want to eventually get a dynamic pattern matching
>> language, but we also want to get it right, we are currently spending our
>> time on the in-language DSL, and especially for the large scale stuff the
>> developers we work with need surprisingly little help (the included example
>> for replacing reduncant c_str() calls was created by a contributor who's not
>> worked on the implementation of the matcher library).
>>
>>
>> I'm not sure I get this logic.  You're saying that you're afraid you won't
>> get the matching language right, so you'd avoiding it and doing something
>> you know is wrong ;-).  I expect much iteration on this, but all that
>> requires is to tell people to expect breakage as you get experience with it
>> and evolve things.
>>
> The main problem with building a dynamic language is that it's
> significantly more effort, as it will require to be a lot more complete
> before being useful.
>
>
> While this is true, "doing the right thing is hard" isn't a good excuse for
> doing the wrong thing :).  I know this isn't what you're actually saying,
> but it's better (to me at least) to try to understand the long term
> direction and then figure out incremental steps to get there.  It's
> generally easier to motivate things if you say that this is a step in the
> right direction.
>

I completely agree.

>  I don't think our current code is "wrong". I think it is useful for a
> certain amount of tools, and as the basis of more dynamic tools.
>
>
> Ok, I agree with you here.  I think that the compile-time pattern matching
> could be very useful, for a broad variety of clients.  Many warnings in the
> compiler are implemented by effectively pattern matching on ASTs, and it
> would be much more nice to use a pattern matching style API instead of a
> manual tree walk.  These should clearly be statically compiled into the
> binary.
>
> 2. You're building substantial new functionality into clang.  The clang
>> binary is already overly bloated with the static analyzer and other
>> functionality that it keeps accreting .  It would be better to use (and
>> improve) the clang plugin mechanism to build this as a plugin.  I'd also
>> like the analyzer to move off to being a plugin as well.  One carrot that we
>> can give for people to build things as plugins is that they can use C++'0x
>> features even though the clang compiler has to stay C++'98 for the
>> forseeable future.
>>
>
> The point why I don't want to run tools as a plugin is that the command
> line syntax for tools can be significantly different from running the
> compiler. I don't think this needs to be linked into the clang binary though
> - as nothing in clang depends on it (confused...)
>
>
> Ok, I think that the root of the issue is that the patch just has too many
> different things going on.  I'm still dubious about the tooling
> infrastructure, but the pattern matching functionality is clear goodness.
>

And I agree with you on the "too many things going on". See below.


>
> How about we start with *just* the AST pattern matching APIs, iterating on
> them until we get to something that can go in, with some warnings switched
> over to using it as an example.  As a step towards this, please take a look
> at include/llvm/Support/PatternMatch.h, which does pattern matching on LLVM
> IR.  I'd prefer the clang matching API to look more similar to it.
>

So, one of the interesting things is that part of the tooling makes it
really easy to run stuff over a piece of code in memory, which all the
testing for the AST matching stuff relies on.
This is why on April 7th I sent out a first patch with a first step in the
tooling:
http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20110404/040694.html
Which I pinged on April 12th:
http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20110411/040826.html
especially asking for negative feedback if anybody thinks this is a stupid
idea.
Since nobody answered until April 21st, I asked Doug in a private mail
whether he's fine with checking this in, to which he answered that I should
go ahead and commit.
The rest of the commits since then went similarly, and were built on top of
the other functionality already checked in. There were some bugfixes etc. on
top of it.
So, that's why I'd like to get the tooling stuff sorted out first. If we can
implement the functionality I need from the tooling in a different way, I'm
listening, but I'd like us to concentrate on that part first, as it makes
all the other things a lot easier to do.

My biggest problem is probably that right now we have divergent work on our
internal version, where we implement feature requests from our internal
customers, and I'd really like do all that development in the open and go to
a model where we backwards-integrate.
The delta is growing bigger over time, and I already spent the effort of
splitting up the patch once, and since I also don't think it makes sense to
split the tests from the matcher code, I currently don't have any free
cycles to do more split-up work that doesn't add value for us.

So, to make a suggestion: I'd propose we revert the revert. Then I'm happy
to spend a portion of my time to massage the code into the end state you
want it to be in, while being able to add the new features that I need to
make my colleagues who are using the tools happy.

To the specific point about llvm/Support/PatternMatcher.h, I looked at this
already when we designed the AST matchers, and I find it looks already very
much like what we propose (apart from the inherent difference in the domain,
and the naming) - perhaps I don't understand enough about the IR to be able
to tell what you would want to see changed there - can you give an example
(perhaps we want to open a new thread on that?)

Thoughts?
/Manuel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20110617/f7cfed59/attachment.html>