Page MenuHomePhabricator

[commands] Support autorepeat in SBCommands
ClosedPublic

Authored by wallace on Apr 3 2020, 5:21 PM.

Details

Summary

This adds support for commands created through the API to support autorepeat.
This covers the case of single word and multiword commands.

Comprehensive tests are included as well.

Diff Detail

Event Timeline

wallace created this revision.Apr 3 2020, 5:21 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 3 2020, 5:21 PM

LGTM. Pavel?

jingham added a subscriber: jingham.Apr 3 2020, 7:53 PM

It's great to add this capability. But if you pass a nullptr, then the exact previous command will be repeated. That's sometimes what you want, but often not. For instance:

(lldb) source list -f foo -l 10
...
(lldb)<RET>

You DON'T want those same source lines to be dumped again.

The full solution would be to add an optional method to the Python class version of the command that returns the "repeat" command string. Then if that is implemented use the string it produces.

There are only a couple of commands (source list being one) that actually have to make up whole new command lines for the repeat command. For most of them - like disassemble - the command with no arguments does "continue from where you left off". So then the you return just your command name for the repeat command. Maybe there's some way to add this convention in without having to go whole hog on exposing the repeat string behavior.

For instance, maybe instead of passing in a bool, you could pass in the repeat command? So when you create the command, if you pass in None, you get no repeat, if you pass in "" you get the exact command line repeated, and if you pass in anything else, that will be the repeat command. Or maybe use an enum with "noRepeat, exactRepeat, commandNameRepeat"?

wallace planned changes to this revision.Apr 3 2020, 8:56 PM

Very good idea!

I was thinking if auto repeat was enabled that the DoExecute would just get called again with nothing and then the plug-in could maintain any repeat commands without needing to add anything... Glad you chimed in!

The biggest issue is maintaining the API. We can't add anything to SBCommandPluginInterface

labath added a comment.Apr 6 2020, 3:10 AM

The biggest issue is maintaining the API. We can't add anything to SBCommandPluginInterface

If we're going to be adding a new inheritable class for this feature, I'd recommend adding about half a dozen or so dummy virtual methods to it so we reserve vtable space for future expansion.

lldb/test/API/api/auto-repeat-command/TestSBCommandAutoRepeat.py
1 ↗(On Diff #254965)

Could you try making this a unit test (like the regular c++ unit tests)?

I know we have already have tests following this pattern, but that was because they were all written before we had googletest, and they all have a lot of problems I'm not keen on repeating.

The biggest issue is maintaining the API. We can't add anything to SBCommandPluginInterface

If we're going to be adding a new inheritable class for this feature, I'd recommend adding about half a dozen or so dummy virtual methods to it so we reserve vtable space for future expansion.

I second that!

wallace updated this revision to Diff 255426.Apr 6 2020, 11:54 AM
  • Moved the test to gtest. It's much better this way and I learned gtest
  • Changed the API. Some notes:

Within the scope of a subcommand, the subcommand doesn't know the parent's
command name. It only knows its own name and it doesn't have any referrence to
its parent. That makes it very difficult to implement an enum option for
eCommandNameRepeat, as @jingham suggested. The GetRepeatCommand signature also
doesn't provide useful info about the parsed arguments.

I took a look at the existing implementations for GetRepeatCommand, and it seems
that most of them just return nullptr, "", or the same command without flags.

I don't want to change any existing core command interpreter function, so I
think that a simple API that covers most of the cases is just providing nullptr
for repeating the same command, "" for not repeating, or a custom string for
any other case. If in the future anyone needs something very customized, a new
override could be created, but I don't think this will happen anytime soon.

Another option is to provide a callback function instead of a string, but I
don't know if it's too much.

labath added a comment.Apr 7 2020, 7:02 AM

Thank you for trying out gtest. I'm very happy that this is working out for you. The test looks fine -- just please move it to unittests/API (new folder) -- that's where it logically belongs as it's testing the API code (and it also avoids dangerous ODR violations resulting from linking in both liblldb and its constituent libraries).

It sounds like you and Jim have mostly agreed on the way this should be implemented. I'll let you two work out any kinks there.

lldb/unittests/Interpreter/TestAutoRepeat.cpp
25 ↗(On Diff #255429)

The recommended llvm style for this is /*arg_name=*/value.

61 ↗(On Diff #255429)

make a gtest assert out of this?

labath added a comment.Apr 7 2020, 7:07 AM

BTW, in gtest tests, the prevailing convention is to name test .cpp files according to the name of the .cpp file under test. So, in this case, that would be something like unittests/API/SBCommandInterpreterTest.cpp. I wouldn't want to take that convention to the extreme and have 10kloc test files, but until we start having more of these tests, I think we can stick to that.

jingham added a comment.EditedApr 7 2020, 4:15 PM

This looks good to me, except that instead of leaving all the other variants of AddCommand, they should funnel through the one that takes the most arguments. It was poor form to leave two around and more so now that there's three.

I'm beginning to think we need an SBAddCommandOptions or something like that so we don't need to add yet another variant next time we want to add some flags to a command. That is a nice-to-have, however. If you are done with this change, it's okay as it stands.

In retrospect, I think it would be better if the CommandInterpreter were to prepend the container commands to the repeat command string. It is bogus that multi-word commands have to know their hierarchy to emit a useful repeat command string.

Prepending the containing commands would mean that command A couldn't invoke a totally different command as its repeat command. But that seems like an odd thing to do anyway. And that way a command wouldn't have to know where it is situated in the command hierarchy for the repeat command to work. That makes your test ugly, but that's not your fault (it's probably some earlier version of me's decision).

lldb/source/API/SBCommandInterpreter.cpp
848

Instead of keeping these copies of AddCommand around, can you funnel all of them through the one that takes the auto_repeat command? That would reduce boilerplate and make adding the next thing easier...

wallace updated this revision to Diff 256027.Apr 8 2020, 8:14 AM

address comments

jingham accepted this revision.Apr 8 2020, 10:29 AM

LGTM

This revision is now accepted and ready to land.Apr 8 2020, 10:29 AM
Closed by commit rGbe3f8a8e1b95: [commands] Support autorepeat in SBCommands (authored by Walter Erquinigo <wallace@fb.com>). · Explain WhyApr 8 2020, 11:24 AM
This revision was automatically updated to reflect the committed changes.
labath added inline comments.Apr 8 2020, 11:46 PM
lldb/unittests/API/CMakeLists.txt
2

Well.. this is now definitely going to get tested well. :P

While we do have some inconsistency where some test files put "Test" as a prefix and others as a suffix, we definitely don't need to have both in the same file. The Test suffix is a prevailing convention (792 vs. 62), so I recommend sticking to that.