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:<br><br><div class="gmail_quote">
On Wed, Jan 18, 2012 at 3:26 AM, James Molloy <span dir="ltr"><<a href="mailto:james.molloy@arm.com">james.molloy@arm.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Firstly, this first patch has some limitations:<br>
  * It has only been fully implemented for Linux, and only tested (to<br>
exhaustion) on Ubuntu Natty and RHEL5.<br>
  * It only deals with what should be the "uncommon" case - loading JSON at<br>
runtime.<br></blockquote><div><br></div><div>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.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Secondly, this is what it does do:<br>
  * llvm.diff adds SMLoc tracking, raw MemoryBuffer input and '// ...' style<br>
comment support to the JSON parser. This makes developing the target DB<br>
files a lot easier, and the comment support is invaluable (and means we can<br>
add the copyright header).<br></blockquote><div><br></div><div>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.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Speaking of tests, the submitted patch does not have any yet. I intend to<br>
submit them in a later patch, as I'd like this to be at least concept<br>
approved before I spend an infinite amount of time writing a decent test<br>
set.<br></blockquote><div><br></div><div>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.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
P.S.: Would people prefer me to commit this now and have post-commit reviews<br>
going on?</blockquote><div><br></div><div>Absolutely not. This is a very invasive change, and needs careful review first.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
 It's quite large so incrementally changing it by review will be a<br>
PITA for reviewers to look through each time (and the functionality is<br>
guarded by flag...)<br></blockquote><div><br></div><div>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.</div>
</div>