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