> On May 23, 2018, at 7:21 PM, Zachary Turner <ztur...@google.com> wrote: > > > On Wed, May 23, 2018 at 7:04 PM Jim Ingham via Phabricator > <revi...@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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
