[Lldb-commits] [PATCH] D114448: [Bug 49018][lldb] Fix incorrect help text for 'memory write' command

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a comment.

Thanks for looking at this bug! I admit I don't know the option parsing very well which is why I shied away from fixing it myself.

Some good finds elsewhere too, but better as their own changes I think. (feel free to add me on review there too)



================
Comment at: lldb/include/lldb/Interpreter/OptionGroupFormat.h:37
+          UINT64_MAX, // Pass UINT64_MAX to disable the "--count" option
+      OptionGroupFormatUsageTextVector usage_text_vector = {});
 
----------------
Would be good to add a comment here, something like:
```
// The default help text is written for "memory read", this can be used to override that
```


================
Comment at: lldb/include/lldb/Interpreter/OptionGroupFormat.h:82
+  std::string m_format_usage_text;
+  std::string m_byte_size_usage_text;
 };
----------------
If the message is a `const char*` do we need to store this in a std::string? I'm, perhaps naively, assuming that you'll only do this override once. Or that if you did it again in a subclassed command, it would replace the whole text.


================
Comment at: lldb/source/Commands/CommandObjectMemory.cpp:1177
+  return usage_text_vec;
+}
+
----------------
Can you do this like this? Saves writing push_back a few times.
```
return { std::make_tuple<...>, ... };
```

Or you could construct this directly in the constructor parameters below. I'm not sure you need a new function for it.
(depends what clang-format makes of it I guess, if it's unreadable then sure have a utility function)



================
Comment at: lldb/source/Commands/CommandObjectMemory.cpp:1255
     value_arg.arg_repetition = eArgRepeatPlus;
+    value_arg.arg_opt_set_association = LLDB_OPT_SET_1;
 
----------------
Which part of this change does this contribute to?


================
Comment at: lldb/source/Commands/CommandObjectMemory.cpp:1299
+        return false;
+      }
     } else if (argc < 2) {
----------------
Great job spotting this, but I'd rather it was in its own change with its own test.


================
Comment at: lldb/source/Interpreter/OptionGroupFormat.cpp:66
+           0,
+           eArgTypeCount,
+           "The number of total items to display."},
----------------
Not sure if clang-format has done this, or just your preferred style. Nothing against either but it makes it difficult to tell if anything has changed in this particular part of the diff.

In general we want clang-formatted code but if it makes the diff tricky to read it's best done in a follow up change that just does formatting.


================
Comment at: lldb/source/Interpreter/OptionGroupFormat.cpp:73
+      case eArgTypeFormat:
+        m_format_usage_text.append(std::get<1>(usage_text_tuple));
+        m_option_definition[0].usage_text = m_format_usage_text.data();
----------------
As I said above, if this is about commands being able to *replace* the help text I don't think append is needed here.

Unless std::string is helping you check whether it's been set or not, which I think you could do with nullptr just as well but please correct me if I'm missing some context.


================
Comment at: lldb/test/API/commands/help/TestHelp.py:246
+                "memory write [-f <format>] [-s <byte-size>] <address> <value> [<value> [...]]",
+                "memory write -i <filename> [-s <byte-size>] [-o <offset>] <address>",
+                "-f <format> ( --format <format> )", "The format to use for each of the value to be written.",
----------------
If these `memory write <options>` lines are checking for the other issue you found, I'd put those in their own test.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114448/new/

https://reviews.llvm.org/D114448