Page MenuHomePhabricator

Break dependency from Expression -> Commands
Needs RevisionPublic

Authored by zturner on May 22 2018, 3:09 PM.

Details

Summary

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 depend on REPL than the other way around. The command library should be thought of as a UI library, low level engine stuff shouldn't depend on it, but the other way around makes perfect sense.

Anyway, only about 3-4 of the fields of this structure were even used and the rest were ignored, so we just give REPL its own custom structure and have the command object fill this out and pass it in. This way REPL doesn't need to reference one from a higher level library.

This breaks the cycle from Expression -> Commands -> Expression, reducing the distinct cycle count to 36.

Note that no CMakeLists.txt file needs to be updated here, because Expression was (incorrectly) not including Commands in its link list to begin with.

Diff Detail

Event Timeline

zturner created this revision.May 22 2018, 3:09 PM
davide accepted this revision.May 22 2018, 4:07 PM

Looks fine to me, but let's wait for Pavel. Are you back working on lldb ? :)

This revision is now accepted and ready to land.May 22 2018, 4:07 PM
jingham requested changes to this revision.May 22 2018, 5:03 PM
jingham added a subscriber: jingham.

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

So it seems odd to handle the passing of one subset of those options from the expression command to this expression mode differently from all the other options you pass to it (e.g. the variable object - i.e. result printing - options.)

This revision now requires changes to proceed.May 22 2018, 5:03 PM

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 using all the OptionGroups would be expected.

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.

lldb/include/lldb/Expression/REPL.h
31

If you stick with this solution, please make this Timeout<std::micro>

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 first in that it pushes an IOHandler to deal with gathering the entered text that the user typed up to the point where they type an empty expression.

The repl adds a third mode:

(lldb) expr -r --

1> let a = 10

a: Int = 10

2> print(a)

10

3>

Which works the same as "expr --toplevel" with the addition of a fancier IOHandler that looks for complete expressions and evaluates each completed expression as it sees it. I'm glossing over a few details here (like the fact that ":" will switch you to the lldb command interpreter), but for the most part that's all it is.

Note, there's another "repl" that lldb provides:

> lldb --repl

Which packages up "make a target out of a little victim program we've stashed away in LLDB.framework, set a breakpoint in it, run to the breakpoint and then execute "expr -r --". So there's also a little bit to support that. But I don't think that materially changes how you should understand the REPL. And then most people actually access this for swift by invoking "swift" with no arguments, but the swift driver with no arguments just turns around and exec's "lldb --repl" so that doesn't add much besides convenience.

Jim

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 using all the OptionGroups would be expected.

Core doesn't currently have a dependency on Commands (few things do actually), and I would prefer not to add one. But several things include REPL.h (notably Debugger.cpp and Target.cpp. So if I move this file to Commands it would introduce 2 extra edges in the dependency graph both of which would introduce a new layering violation.

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.

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.

labath resigned from this revision.Fri, Aug 9, 2:05 AM