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

[Dave Lee via Phabricator via lldb-commits]

kastiglione added inline comments.


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


================
Comment at: lldb/source/Commands/CommandObjectPlatform.cpp:1627
+
+        if (!FileSystem::Instance().Readable(option_arg)) {
+          error.SetErrorStringWithFormat("File \"%s\" is not readable.",
----------------
Is it necessary to check readability? If not done here, at execution time would a non-readable interpreter lead to a useful error message?

If checking for readability is desirable here, is checking that it's executable also desirable?


================
Comment at: lldb/test/API/commands/platform/basic/TestPlatformCommand.py:109
+        self.assertTrue(err.Success())
+        self.assertIn("/bin/sh", sh_cmd.GetOutput())
+
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86667