Page MenuHomePhabricator

[lldb/interpreter] Move the history subcommand to session (NFCI)
ClosedPublic

Authored by mib on Jul 22 2020, 4:23 AM.

Details

Summary

This patch moves the history subcommand from the command to session command.
I think it makes more sense to have it there because as the command usage
suggests, it should be used to manage custom LLDB commands.

However, history is essentially tied to a debugging session and holds
all the commands (not specifically custom ones).

This also makes it more discoverable by adding an alias for it (mimicking the shell builtin).

Signed-off-by: Med Ismail Bennani <medismail.bennani@gmail.com>

Diff Detail

Event Timeline

mib created this revision.Jul 22 2020, 4:23 AM
mib edited the summary of this revision. (Show Details)Jul 22 2020, 4:34 AM

I don't have any strong opinions about this either way. The history alias is kind of nice because it matches the bash builtin, but I don't use either of them often enough to care.

Isn't session just concerned with the *current* LLDB session? LLDB's command history is only spanning the current session at the moment, but I always thought that's more of a bug and not a feature (In fact one of my GSoC students may actually have to implement that the history is spanning multiple sessions). On the other hand it seems to best fit the current behaviour so it's fine by me.

teemperor removed a subscriber: jingham.

(Phab send the whole thing when I pressed enter....)

I added Jim who might have more insights on the design rationale here, but IMHO this seems like an improvement.

The current lldb behavior is that when you start up lldb, it reads the editline command-history file (so the history of the previous session), and loads it into it's internal command history, which you can get to by up and down arrows or ^R searches. But it doesn't prime the list that the "command history" command has access to. So command history starts out empty. But IMO, that's just bug. If a command shows up in scrolling through with up-arrow, it should also be listed in the history command wherever it lives. This really reduces the utility of the "history" command... I think we do want to show this. We might want mark in the "command history" output where the imported commands end and the new commands start, but we really should make them available.

However, even if we started doing this right, I think the list of historical commands that were imported into lldb at startup properly belong to the current session. So I don't think this really argues either way.

I'm fine with history going under session.

I didn't hit "accept revision" because I only addressed Raphael's question, I haven't read the patch yet. But if somebody else has and is okay with the details, I have no objections to it.

mib edited the summary of this revision. (Show Details)Jul 22 2020, 12:28 PM
teemperor accepted this revision.Jul 23 2020, 3:24 AM

The patch itself is fine, so let's ship it!

This revision is now accepted and ready to land.Jul 23 2020, 3:24 AM
This revision was automatically updated to reflect the committed changes.