[lldb-dev] large patches vs incremental changes

[Greg Clayton]

> On Jul 1, 2014, at 12:01 PM, Zachary Turner <zturner at google.com> wrote:
> 
> I wanted to open a discussion about the policy regarding patch submission.
> 
> LLVM, and most of its other subprojects, have adopted a policy of making small, incremental changes, as outlined here [http://llvm.org/docs/DeveloperPolicy.html#making-a-major-change].   It seems that either we don't have this on LLDB, or we do have it and it's just not enforced.
> 
> I personally have a strong preference for the incremental approach.  All the reasons are outlined at the above link, so I don't think I need to repeat them.  Is it possible to move towards this model with LLDB?

Incremental is the preferred approach just like LLVM.
> 
> Additionally, I frequently see patches going in without review.  What is LLDB's policy on this?  

It really depends on the people doing the committing. We have 5 main people on LLDB here at Apple and we all commit without review as we developed LLDB from the ground up and understand the architecture. We also internally will review each other's patches as they go in and ask each other to fix things, so feel free to ask questions and get clarifications as you see commits going in and you want to understand things.

For newer contributors we prefer to review patches for a while until we can trust that the patches going in make sense and follow the architecture and design patterns we have used.

> Are we're ok with having broken code upstreamed for a short time on the condition that the committer is acting in good faith to fix it as soon as possible?  

Yes, I am not expecting everyone to build for unix, MacOSX, and Windows. If anything breaks things should be fixed ASAP. Sometimes it is easy for people to fix any build issues that come up, and other times the fix is less obvious. For header inclusions and simple build breakages these should be fixed quickly without intervention. Ideally we need to have buildbots on all systems: MacOSX, FreeBSD, Linux, and Windows and anytime anything breaks the buildbots lets users know and things get fixed.

> For example, I've had a patch up for 6 days that I've pinged on a couple of times, with no traction.  

We see these patches and we need our experts in the field to review them and our local reviewers need to keep up better with this new traffic. We have some time constraints here at Apple that are keeping us busy, so we have a lot on our plate so please be patient.

> I've been waiting because I was under the impression that getting a review was a requirement.  

Again, for newer contributors we do prefer getting a review.

> But sometimes I see patches that seem non-trivial getting pushed straight to the server without any kind of review.  So I'm not sure if my understanding is correct.  Either way, some clarification woudl be nice.

So long term contributors have the ability to commit with no review.

The areas of expertise are as follows:

Greg Clayton: DWARF parser, ObjectFile, SymbolFile, plug-in architecture, public API, types, overall design and architecture, ValueObject, Python, IOHandlers, debugserver, overall process debugging, ARM architectures, x86 architectures
Jim Ingham: Thread stepping plans, architecture, overall design and architecture, ValueObject, overall process debugging
Sean Callanan: expression parser, clang IR emulation, JIT, disassembler
Enrico Granata: type format, type summaries, type synthetic, data formatters, ValueObject, Python, Python bridging
Jason Molenda: stack backtracing, process queues, disassembler, ARM architectures, x86 architectures

Your latest patches are mostly on the expression parser side and I need Sean Callanan to comment on anything that goes into the expression parser, IR memory map. Sean has been really busy getting other stuff done and I have asked him to make time to get your patches reviewed. Sean is out today and will be back tomorrow.

I hope this helps clear things up a bit, let us know if you have any more questions.

Greg Clayton