[cfe-commits] [PATCH] MS style inline assembly support

[Chandler Carruth]

On Mon, Jun 11, 2012 at 2:28 PM, Chad Rosier <mcrosier at apple.com> 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...
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120611/e7fa258e/attachment.html>