<div><br><div class="gmail_quote"><div dir="auto">On Wed, May 23, 2018 at 7:04 PM Jim Ingham via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">jingham added a comment.<br>
<br>
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.</blockquote><div dir="auto"><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"></blockquote><div dir="auto">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</div><div dir="auto"><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
<br>
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.<br>
<br>
That would be fine, and then you could leave REPL.cpp where it is.</blockquote><div dir="auto"><br></div><div dir="auto">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.  </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"></blockquote></div></div>