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

[Zachary Turner via lldb-commits]

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


>
> 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.

>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20180523/c4e86aae/attachment.html>