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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I am wondering what should the platform classes which do not implement this feature do (although most of them could implemented I don't think you need to implement all of them -- PlatformGdbRemote in particular can be a bit tricky). It seems to me like it would be better for them to bail out if a non-standard shell is requested instead of silently executing a command the user did not intend.



================
Comment at: lldb/include/lldb/Target/Platform.h:633
+      const Timeout<std::micro> &timeout,
+      const bool run_in_default_shell = true);
 
----------------
const on a value parameter is useless. Also, default arguments on virtual methods are somewhat shoot-footy: <http://www.cplusplus.com/forum/general/69970/>. The [[ https://en.wikipedia.org/wiki/Non-virtual_interface_pattern | Non-virtual interface pattern ]] would solve that, and maybe have some other benefits.


================
Comment at: lldb/source/API/SBPlatform.cpp:59-68
+    if (command_interpreter && command_interpreter[0]) {
+      full_command += command_interpreter;
+      full_command += " -c ";
+    }
+
+    if (shell_command && shell_command[0]) {
+      full_command += " \"";
----------------
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.


================
Comment at: lldb/source/Commands/CommandObjectPlatform.cpp:1621-1633
+        if (!FileSystem::Instance().Exists(option_arg)) {
+          error.SetErrorStringWithFormat("File \"%s\" doesn't exist.",
+                                         option_arg.str().c_str());
+          return error;
+        }
+
+        if (!FileSystem::Instance().Readable(option_arg)) {
----------------
This won't work for remote platforms. The checks should be at least done in the Platform class, but ideally I would really just leave it to the operating system to figure out if it can or cannot run a given command.


================
Comment at: lldb/test/API/commands/platform/basic/TestPlatformCommand.py:96
+
+    @expectedFailureAll(oslist=["windows"])
+    @no_debug_info_test
----------------
You could build a small executable to serve as the "shell". Then, this test would work everywhere.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86667