[Lldb-commits] [PATCH] D47232: Break dependency from Expression -> Commands

[Jim Ingham via lldb-commits]

> On May 23, 2018, at 7:21 PM, Zachary Turner <zturner at google.com> wrote:
> 
> 
> On Wed, May 23, 2018 at 7:04 PM Jim Ingham via Phabricator <reviews at reviews.llvm.org> wrote:
> jingham added a comment.
> 
> I worry when concerns of layering which seem a little artificial to me would make us add a shadow class for something that is already well expressed as it is.  If you pass them as arguments, and then I add another useful one and want to use it in both the regular and the REPL versions of the expression parser, it would be ugly to have to add another argument to this function when I should have been able to share the structure.
> 
> I don’t think the concerns are artificial.  Layering violations prevent lldb from being used as a library and pulling in only the parts you need.  In addition, they actually prevent a lot of our internal tooling from working well so it’s a high priority for us to fix all of them.  Finally, resolving them and achieving proper layering improves build parallelism which benefits all developers, not to mention the build infrastructure and bots
> 
> 

I see the point of having the code that is being shared between independent entities like lldb-server and lldb proper bring in as little of lldb as possible, since lldb-server needs to be a light-weight tool that isn't trying to be a debugger.  And there are other clearly separable bits like the DWARF parser of object file readers that could be shared.  But for instance I don't think we are interested in providing a generic command interpreter.  That's a potentially interesting job but not one that we have the resources to take on.  And I don't thing we would want a Debugger that might or might not have a command interpreter.  So teasing apart dependencies there seem more a formal than an actual concern.

> 
> Seems like the thing you are actually objecting to is the use of CommandObjectExpression::CommandOptions in REPL.h.  REPL.h also use OptionGroupValueObjectDisplay or OptionGroupFormat.  They live in Interpreter, which this and other files in Expression have to use for other reasons as well as to get these option groups.  Their usage doesn't change your graph.  So another way to address your concerns would be to make an OptionGroupExpressionOptions in source/Interpreter and have CommandObjectExpression and REPL.h use that.  There's no reason other than convenience that the option group for expression specific options live in the CommandObjectExpression class.
> 
> That would be fine, and then you could leave REPL.cpp where it is.
> 
> Isn’t that quite a bit more complicated than just creating a new structure as I’ve done?  I think the original patch is pretty simple and straightforward.  I’d prefer a simple solution that has low impact and gets the job done over one that tacks on a bunch of additional machinery that YAGN.  

But your new structure adds an impedance layer where there needn't be one, and I don't think the principle of adding odd little shims to achieve your goal of layering separation is a good one.

Jim