[cfe-commits] [PATCH] Driver reengineering - first patch

[Chandler Carruth]

I'm starting to work through this patch (and some of your others) but I
wanted to give some immediate feedback on a few things that I think are
going to make reviewing and committing this very hard:

On Wed, Jan 18, 2012 at 3:26 AM, James Molloy <james.molloy at arm.com> wrote:

> Firstly, this first patch has some limitations:
>  * It has only been fully implemented for Linux, and only tested (to
> exhaustion) on Ubuntu Natty and RHEL5.
>  * It only deals with what should be the "uncommon" case - loading JSON at
> runtime.
>

As I have mentioned to you before, I think this is the fundamentally wrong
way to build this. It makes it very hard to review, and essentially
impossible to test. I would much rather you handled the common cases first,
especially as that would have resulted in significantly more incremental
patches.

Secondly, this is what it does do:
>  * llvm.diff adds SMLoc tracking, raw MemoryBuffer input and '// ...' style
> comment support to the JSON parser. This makes developing the target DB
> files a lot easier, and the comment support is invaluable (and means we can
> add the copyright header).
>

Have you coordinated with Michael Spencer about this? He is already working
on many of the same issues, and has a more principled approach based on the
YAML spec. I would prefer if we could follow that rather than adding
arbitrary extensions to the JSON parsing.


> Speaking of tests, the submitted patch does not have any yet. I intend to
> submit them in a later patch, as I'd like this to be at least concept
> approved before I spend an infinite amount of time writing a decent test
> set.
>

I think decent tests are a must -- they will help reviewers understand how
your patch works. You shouldn't need an infinite amount of time, or build a
complete set, but please start producing tests that can be committed with
the functionality they exercise.

P.S.: Would people prefer me to commit this now and have post-commit reviews
> going on?


Absolutely not. This is a very invasive change, and needs careful review
first.

It's quite large so incrementally changing it by review will be a
> PITA for reviewers to look through each time (and the functionality is
> guarded by flag...)
>

This is a fundamental problem with how you have proposed the patch, and is
something I think you need to address and deal with. Can you instead factor
it into small, incremental changes? That helps both you maintain it, and
the reviewers understand it. I think it will be very hard to get it
reviewed and committed given its size and complexity.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120120/ccdb4629/attachment.html>