[Lldb-commits] [PATCH] D149472: [lldb] Refactor host::OpenFileInExternalEditor

[Alex Langford via Phabricator via lldb-commits]

bulbazord added inline comments.


================
Comment at: lldb/source/Host/macosx/objcxx/Host.mm:337
+
+  LLDB_LOG(log, "Sending {0}:{1} to external editor", file_path, line_no);
+
----------------
JDevlieghere wrote:
> bulbazord wrote:
> > nit: Move log line below checking if the file_path is empty? If the file_spec is empty we may see strange log lines like "Sending :0 to external editor" which is just noise.
> I actually did that on purpose, so we could tell from the logs that it's empty. It didn't seem worth adding a whole separate log line for, but if you think `:10` looks weird, I'm happy to do it. 
I think a separate log line would be easier to read for somebody not familiar with this codepath. It would be confusing otherwise. Something like `"OpenFileInExternalEditor called with empty path"`? Or maybe you can keep the log line but change it to:

```
LLDB_LOG(log, "Sending {0}:{1} to external editor", file_path.empty() ? "<invalid>" : file_path, line_no);


================
Comment at: lldb/source/Host/macosx/objcxx/Host.mm:387-388
+  static std::once_flag g_once_flag;
+  std::call_once(g_once_flag, [&]() {
+    if (const char *external_editor = ::getenv("LLDB_EXTERNAL_EDITOR")) {
+      LLDB_LOG(log, "Looking for external editor: {0}", external_editor);
----------------
JDevlieghere wrote:
> bulbazord wrote:
> > I know you're preserving existing behavior here (or rather, fixing its faulty implementation), but I think it would be nice if you didn't have to restart your session and add an environment variable to get this working with an existing debug session. Or maybe make it a setting?
> I think we're trying to mimic the `EDITOR` and `VISUAL` environment variables but I agree a setting would be better. I can add that in a follow-up. Which one should take preference if you have both?
I would say a setting should probably take priority? That way you can change it during your session if desired.


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

https://reviews.llvm.org/D149472