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

[Chris Lattner]

Hi Manuel,

On Jun 17, 2011, at 11:40 AM, Manuel Klimek wrote:
> 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.

I completely agree with you that this was poorly handled, I'm honestly sorry for that.

> 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.

I'll say up front that this is completely a policy decision, not a technical one.  I understand that you're trying to solve an immediate problem and move on to new and more interesting challenges.

>From my perspective, I care more about the long term health of the project than I do about any one individual small feature if they aren't implemented 'right' (whatever that means :) .  The reason for this is that an aggregation of small features increases the size of the code if not factored right and integrated in the right way, causing it to be more difficult to change and improve the code.

We've also had experience (e.g. the XML dumper) where code was contributed to the project with the promise that it would "get fixed later"... and later never came.  As a project lead in an open source project, I only have power in two ways: 1) preventing code from going in and 2) asking that code be removed, so that it is correct when it goes in.  

Allowing you to commit the code in a way that cannot be practically reviewed and whose design is already known to be questionable is asking to *completely* subvert our code review process, which is not acceptable.  I'm sorry that this is causing pain for you, but unfortunately, this is how it has to be.

> 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.

There are two major problems with this approach:

1) you could get hit by a bus, distracted by your manager, or otherwise lose interest, thus never actually fixing the problem.  I hope that this doesn't happen, but stranger things have happened.  

2) this makes it impossible to understand the diff.  As a hypothetical, lets say someone committed a patch that (unknown) had 10 problems in it.  They continued hacking on the code, and managed to fix 8 of the problems.  We would have lost the ability to know the entire surface area of the patch, thus making it impossible for code review to fix those 2 remaining issues.

This is why we have a policy that all code that goes in the tree be code reviewed by code owners (though it can happen before or after commit).  Breaking our process because it is inconvenient and because the earlier reviews were mishandled unfortunately isn't an option.

> 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?)

My primary concern is the naming issues.

-Chris

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20110624/018795b6/attachment.html>