[Lldb-commits] [PATCH] D47232: Break dependency from Expression -> Commands
Zachary Turner via lldb-commits
lldb-commits at lists.llvm.org
Wed May 23 19:21:29 PDT 2018
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
> 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
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the lldb-commits