[Lldb-commits] [PATCH] D114544: [lldb] Fix 'memory write' to not allow specifying values when writing file contents

[David Spickett via Phabricator via lldb-commits]

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