DavidSpickett added inline comments.
================
Comment at: lldb/source/Interpreter/CommandObject.cpp:457
: OptSetFiltered(opt_set_mask, m_arguments[i]);
+ if (arg_entry.empty())
+ continue;
----------------
(like I said on the other diff)
What does it mean for an arg_entry to be empty? (please add the answer as a
comment in the code as well)
I think this is avoiding trying to index [0] on an empty arg lower down:
```
} else {
StreamString names;
for (int j = 0; j < num_alternatives; ++j) {
if (j > 0)
names.Printf(" | ");
names.Printf("%s", GetArgumentName(arg_entry[j].arg_type));
}
std::string name_str = std::string(names.GetString());
switch (arg_entry[0].arg_repetition) {
```
Which is fine but I'm not sure if arg_entry being empty is unusual enough to
prefer an assert rather than skipping over it.
================
Comment at: lldb/test/API/commands/memory/write/TestMemoryWrite.py:1
+"""
+Test the 'memory write' command.
----------------
I wasn't expecting such thorough tests, this is great!
================
Comment at: lldb/test/API/commands/memory/write/TestMemoryWrite.py:49
+
+ # (lldb) memory read --format c --size 7 --count 1 `&my_string`
+ # 0x7fff5fbff990: abcdefg
----------------
You don't need to add these comments. The output is clear enough from the
`expect` parameters.
================
Comment at: lldb/test/API/commands/memory/write/TestMemoryWrite.py:57
+ self.expect(
+ "memory write --format c --size 7 `&my_string` ABCDEFG",
error=False)
+
----------------
I think `error=False` is the default so you can leave it out here. We tend to
add `error=True` for the few times we want a failure.
================
Comment at: lldb/test/API/commands/memory/write/TestMemoryWrite.py:81
+ "memory write --infile file.txt --size 7 `&my_string` ABCDEFG",
error=True,
+ substrs=['memory write takes only a destination address when
writing file contents'])
+
----------------
So here I'd remove the comment and add "error: " to the start of the substr.
================
Comment at: lldb/test/API/commands/memory/write/TestMemoryWrite.py:94
+ "help memory write",
+ matching=True,
+ substrs=[
----------------
Again this is the default, the vast majority of the time we want a match so we
only call out the few where we don't.
================
Comment at: lldb/test/API/commands/memory/write/TestMemoryWrite.py:96
+ substrs=[
+ "Command Options Usage:",
+ "memory write [-f <format>] [-s <byte-size>] <address> <value>
[<value> [...]]",
----------------
You could skip this line. Small thing but we're not testing the general usage
printing infrastructure here just the specifics in the following 2 lines.
================
Comment at: lldb/test/API/commands/memory/write/main.cpp:1
+#include <stdio.h>
+#include <stdint.h>
----------------
Trivial thing, but this file has no need to be .cpp since it's plain c.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D114544/new/
https://reviews.llvm.org/D114544
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
