<div class="gmail_quote">On Fri, Jun 17, 2011 at 10:22 AM, Chris Lattner <span dir="ltr"><<a href="mailto:clattner@apple.com" target="_blank">clattner@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<div style="word-wrap:break-word"><div><div><div>On Jun 14, 2011, at 11:21 PM, Manuel Klimek wrote:</div><blockquote type="cite"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0.8ex;border-left-width:1px;border-left-color:rgb(204, 204, 204);border-left-style:solid;padding-left:1ex">

<div style="word-wrap:break-word"><div><div><blockquote type="cite"><div class="gmail_quote"><div>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).</div>


</div></blockquote><div><br></div></div><div>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.</div>

</div></div></blockquote></div></blockquote></div><div><blockquote type="cite"><div class="gmail_quote"><div>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.</div>

</div></blockquote><div><div class="gmail_quote"><div><br></div></div></div></div><div>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.</div>

</div></div></blockquote><div><br></div><div>I completely agree. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div>
<blockquote type="cite"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0.8ex;border-left-width:1px;border-left-color:rgb(204, 204, 204);border-left-style:solid;padding-left:1ex">

<div style="word-wrap:break-word"><div>
</div></div></blockquote><div>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.</div></div></blockquote><div><br></div></div>

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

<div><div><br></div><blockquote type="cite"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0.8ex;border-left-width:1px;border-left-color:rgb(204, 204, 204);border-left-style:solid;padding-left:1ex">

<div style="word-wrap:break-word"><div>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. </div>


</div></blockquote><div><br></div><div>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...)</div>


</div></blockquote><div><br></div></div><div>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.</div>

</div></div></blockquote><div><br></div><div>And I agree with you on the "too many things going on". See below.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<div style="word-wrap:break-word"><div><div><br></div><div>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.</div>

</div></div></blockquote><div><br></div><div>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.</div>

<div>This is why on April 7th I sent out a first patch with a first step in the tooling: </div><div><a href="http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20110404/040694.html" target="_blank">http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20110404/040694.html</a></div>

<div>Which I pinged on April 12th:</div><div><a href="http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20110411/040826.html" target="_blank">http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20110411/040826.html</a></div>

<div>especially asking for negative feedback if anybody thinks this is a stupid idea.</div><div>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.</div>

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

<div><br></div><div>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.</div>

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

<div><br></div><div>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.</div>
<div><br></div><div>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?)</div>

<div><br></div><div>Thoughts?</div><div>/Manuel</div><div><br></div></div>