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

[Jim Ingham via Phabricator via lldb-commits]

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.

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.


https://reviews.llvm.org/D47232