[Lldb-commits] [PATCH] D117004: [lldb] Don't print "Command Options Usage:" for an alias with no options

[Jim Ingham via Phabricator via lldb-commits]

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