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

Thu Aug 27 21:52:01 PDT 2020

mib marked 9 inline comments as done.
mib added a comment.

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**.

================
Comment at: lldb/source/API/SBPlatform.cpp:59
+
+    if (command_interpreter && command_interpreter[0]) {
+      full_command += command_interpreter;
----------------
labath wrote:
> kastiglione wrote:
> > JDevlieghere wrote:
> > > Given that this pattern repeats a few times in this struct, maybe a small static helper function would be nice:
> > > 
> > > ```static bool is_not_empty(cont char* c) { return c && c[0]; }```
> > Can these be `StringRef`, and then check with `empty()`?
> Rather than duplicating this logic here (and getting it wrong for windows, for nested quotes, etc.) it would be better to just pass the custom shell to the `RunShellCommand` method. Then it use the existing quoting logic, and all that needs to change is that it now picks up the shell from the argument (if passed) instead of the baked-in default.
I think since this method will eventually interact with the Python API, they need to be C strings.

================
Comment at: lldb/test/API/commands/platform/basic/TestPlatformCommand.py:109
+        self.assertTrue(err.Success())
+        self.assertIn("/bin/sh", sh_cmd.GetOutput())
+
----------------
kastiglione wrote:
> 
Since it's a command output, it has a newline character at the end of it.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86667