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