<div class="gmail_quote">On Mon, Jun 11, 2012 at 2:28 PM, Chad Rosier <span dir="ltr"><<a href="mailto:mcrosier@apple.com" target="_blank">mcrosier@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"><br><div><div class="im"><div>On Jun 11, 2012, at 1:56 PM, Chandler Carruth wrote:</div><br><blockquote type="cite">I see this already landed in r158325, but I have comment:<div><br></div>
<div>Why are there no tests??</div><div><br></div><div>It seems unreasonable to add this much code without tests. It also feels like this could be broken up more.</div></blockquote><div><br></div></div><div>I've been using the tests from the llvm-gcc test suite, but didn't convert them into something that worked with the testing infrastructure.  I will do so now and I apologize for not doing this in the first place.</div>
<div class="im"><br><blockquote type="cite">
<div><br></div><div>Why is libclang and RAV support tied to the initial patch? Couldn't we start parsing these first, with test cases, layer on AST representations and Sema with more tests, and finally visit them? We could also defer the serialization stuff... anyways, in the future, I think it might be nice to commit these new features incrementally, with tests for each stage.</div>
</blockquote><div><br></div></div><div>They don't have to be.  My approach for this first patch, which I'm guessing you don't agree with, was to stub out most the components.  I did this more so as a means of learning the code base then following strict coding paractices as this is unfamiliar territory to me.  Kind of a "forest for the trees" approach.  However, moving forward I will be taking a more incremental approach.</div>
</div></div></blockquote><div><br></div><div>For future reference, I'm not really opposed to stubbing stuff out, I do the same and for the same reasons. Mostly my goal is to actually commit things in smaller chunks, and based around the inherent layering. I usually maintain the pure stubs out-of-tree until I can at least get test cases that cover the stub (IE, a synthetic error is a perfectly fine test case to my eyes -- it ensures we actually hit the code path). Anyways, just for future reference. It also makes the detailed post-commit review a bit easier to follow.</div>
<div><br></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 class="im"><br><blockquote type="cite">
<div><br></div><div>It seems somewhat worrisome to commit to libclang support and enshrine its API now, when the AST design sounds very much in flux.</div></blockquote><div><br></div></div><div>We could certainly take a table-driven approach and remove the dependency upon libclang, but that would be largely throw away work.</div>
</div></div></blockquote><div><br></div><div>Hmm, I'm not really suggesting anything is wrong about having this in libclang eventually... I'm not trying to make any comment on the design one way or the other.</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>  I also understand that we don't want an API that is a moving target.</div>
</div></div></blockquote><div><br></div><div>Its this that I was driving at -- we promise that the C API will be largely stable and backwards compatible. Technically speaking that's just from release-to-release, but so far we've been very conservative even between releases with breaking changes to it. Doug or one of the real libclang gurus would probably be a better person to comment here though...</div>
</div>