On Mon, Jun 11, 2012 at 2:28 PM, Chad Rosier <[email protected]> wrote:
> > On Jun 11, 2012, at 1:56 PM, Chandler Carruth wrote: > > I see this already landed in r158325, but I have comment: > > Why are there no tests?? > > It seems unreasonable to add this much code without tests. It also feels > like this could be broken up more. > > > 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. > > > 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. > > > 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. > 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. > > It seems somewhat worrisome to commit to libclang support and enshrine its > API now, when the AST design sounds very much in flux. > > > We could certainly take a table-driven approach and remove the dependency > upon libclang, but that would be largely throw away work. > 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. > I also understand that we don't want an API that is a moving target. > 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...
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
