jingham accepted this revision.
jingham added a comment.
I was confused by your example of the "quit" command since the previous code
output the banner before checking the NumCommandOptions, so that shouldn't have
worked. Actually, there's a check in CommandObject::GenerateHelp for a null
return from GetOptions before this GenerateOptionUsage gets called, which
bypasses the call. Anyway, with this change we'll do this correctly even if we
get here in some other way, so that's all good.
I also think this function is pretty confusing to read. First off,
only_print_args is a really confusing name for "is_dash_dash_command", which is
what that bool really signifies. Also, we do a bunch of checking for both
only_print_args and !only_print_args inside the first "if (!only_print_args)"
block, but that variable is a const so that's just weird.
So far as I can see, the only thing we do in the `only_print_args == true` case
is this block:
if (cmd && (only_print_args || cmd->WantsRawCommandString()) &&
arguments_str.GetSize() > 0) {
if (!only_print_args)
strm.PutChar('\n');
strm.Indent(name);
strm << " " << arguments_str.GetString();
}
plus resetting the indentation.
This function would be much easier to read if we handle IsDashDash commands as
an early return, emitting the command name and arguments, and returning. Then
we could remove of both the if (!only_print_args) blocks, and the weird checks
inside those blocks, and this would make more sense. That little snippet is
sufficiently obvious I don't think repeating it would be a big deal.
OTOH, you didn't originally write this code, so if you don't have time to clean
it up further, then this LGTM.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D117004/new/
https://reviews.llvm.org/D117004
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
