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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
