[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...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20180523/c4e86aae/attachment.html>

More information about the lldb-commits mailing list