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

2018-05-24 Thread Jim Ingham via lldb-commits
> On May 23, 2018, at 7:21 PM, Zachary Turner wrote: > > > On Wed, May 23, 2018 at 7:04 PM Jim Ingham via Phabricator > 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 alre

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

2018-05-23 Thread Zachary Turner via lldb-commits
On Wed, May 23, 2018 at 7:04 PM Jim Ingham via Phabricator < revi...@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 pa

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

2018-05-23 Thread 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 th

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

2018-05-23 Thread Zachary Turner via Phabricator via lldb-commits
zturner added a comment. FWIW `REPL` only uses 4 of the 10+ fields of `CommandObjectExpression::Options`. If you don't like the separate struct idea, I can just pass them directly as as independent arguments to `REPL::SetCommandOptions`. https://reviews.llvm.org/D47232 ___

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

2018-05-23 Thread Zachary Turner via Phabricator via lldb-commits
zturner added a comment. In https://reviews.llvm.org/D47232#1108865, @jingham wrote: > Perhaps a better way to handle this is to think of REPL.cpp/h as adjuncts of > CommandObjectExpression.cpp/h - they mostly define the fancy IOHandler that > gets pushed when you run the command in this mode.

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

2018-05-23 Thread Jim Ingham via Phabricator via lldb-commits
jingham added a subscriber: zturner. jingham added a comment. The expression command had two modes before introducing the REPL. You can do: (lldb) expr -- some C expression or you can do: (lldb) expr Enter expressions, then terminate with an empty line to evaluate: 1: first C expression 2

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

2018-05-23 Thread Jim Ingham via lldb-commits
The expression command had two modes before introducing the REPL. You can do: (lldb) expr -- some C expression or you can do: (lldb) expr Enter expressions, then terminate with an empty line to evaluate: 1: first C expression 2: Second C Expression 3: The second only differs from the fi

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

2018-05-23 Thread Pavel Labath via Phabricator via lldb-commits
labath added a comment. Both of the solutions sound plausible to me (extra struct vs. moving REPL to `Command`). Maybe that's because I don't know enough about the REPL to have formed an opinion on it. Comment at: lldb/include/lldb/Expression/REPL.h:31 +bool TryAllThreads

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

2018-05-22 Thread Jim Ingham via Phabricator via lldb-commits
jingham added a comment. Perhaps a better way to handle this is to think of REPL.cpp/h as adjuncts of CommandObjectExpression.cpp/h - they mostly define the fancy IOHandler that gets pushed when you run the command in this mode. I think it would be fine to move them to Command, at which point

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

2018-05-22 Thread Jim Ingham via Phabricator via lldb-commits
jingham requested changes to this revision. jingham added a comment. This revision now requires changes to proceed. The REPL is just a mode of the expression command. You invoke it by saying: (lldb) expr --repl -- or you can add any other options to it that you want, including flags like -i or

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

2018-05-22 Thread Davide Italiano via Phabricator via lldb-commits
davide accepted this revision. davide added a comment. This revision is now accepted and ready to land. Looks fine to me, but let's wait for Pavel. Are you back working on lldb ? :) https://reviews.llvm.org/D47232 ___ lldb-commits mailing list lldb-

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

2018-05-22 Thread Zachary Turner via Phabricator via lldb-commits
zturner created this revision. zturner added reviewers: labath, davide. The REPL (which lives in Expression) was making use of the command options for the expression command. It's arguable whether `REPL` should even live in `Expression` to begin with, but it makes more sense for Command to depe