[Lldb-commits] [PATCH] D86667: [lldb/Target] Add custom interpreter option to `platform shell`

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D86667#2243781 <https://reviews.llvm.org/D86667#2243781>, @mib wrote:

> In D86667#2242566 <https://reviews.llvm.org/D86667#2242566>, @jingham wrote:
>
>> Do people really call command-line shells interpreters?  I would have thought --shell would be a better name.  lldb already has its own command interpreter which is orthogonal to the shells, so it seems confusing to use the same term for both.
>
> I think `platform shell --shell` sounds/looks repetitive so I opted for `-i|--interpreter` instead, as in Command-line **Interpreter**.

Yes, it is repetitive. OTOH:

- the unix commands that take a shell as an argument (e.g., `su`, `sudo`), do so via a --shell/-s argument
- we have a `memory load --load` command

Overall, I can understand where you're coming from, but I think I'd go with --shell nonetheless...



================
Comment at: lldb/include/lldb/Target/Platform.h:643
+          *command_output, // Pass nullptr if you don't want the command output
+      const Timeout<std::micro> &timeout, bool run_in_shell = true);
 
----------------
The run_in_shell argument makes the inferface weird (why do I need to specify a shell, if I don't want to use it). And still suffers from the virtual default argument problem described above. Let's not propagate this Host weirdness into the platform class.


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp:2820
 lldb_private::Status GDBRemoteCommunicationClient::RunShellCommand(
-    const char *command, // Shouldn't be NULL
+    llvm::StringRef command, // Shouldn't be NULL
     const FileSpec &
----------------
These `Shouldn't be NULL` are out of place now, and probably not needed anymore.


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp:2831
   stream.PutCString("qPlatform_shell:");
-  stream.PutBytesAsRawHex8(command, strlen(command));
+  stream.PutBytesAsRawHex8(command.str().c_str(), command.size());
   stream.PutChar(',');
----------------
`command.data()` will do just fine


================
Comment at: lldb/test/API/commands/platform/basic/TestPlatformPython.py:95
+        executable = self.getBuildArtifact('myshell')
+        d = {'C_SOURCES': 'myshell.c', 'EXE': executable}
+        self.build(dictionary=d)
----------------
We normally set these things in the makefile. The environment tricks are reserved for cases where you need to do something funny (e.g. build multiple things from a single makefile, or similar...).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86667