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

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere marked 3 inline comments as done.
JDevlieghere 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);
+
----------------
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. 


================
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);
----------------
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?


================
Comment at: lldb/source/Host/macosx/objcxx/Host.mm:411-412
       kLSLaunchDefaults | kLSLaunchDontAddToRecents | kLSLaunchDontSwitch;
-
-  char *external_editor = ::getenv("LLDB_EXTERNAL_EDITOR");
-
-  if (external_editor) {
-    LLDB_LOGF(log, "Looking for external editor \"%s\".\n", external_editor);
-
-    if (g_app_name.empty() ||
-        strcmp(g_app_name.c_str(), external_editor) != 0) {
-      CFCString editor_name(external_editor, kCFStringEncodingUTF8);
-      error = ::LSFindApplicationForInfo(kLSUnknownCreator, NULL,
-                                         editor_name.get(), &g_app_fsref, NULL);
-
-      // If we found the app, then store away the name so we don't have to
-      // re-look it up.
-      if (error != noErr) {
-        LLDB_LOGF(log,
-                  "Could not find External Editor application, error: %ld.\n",
-                  error);
-        return false;
-      }
-    }
-    app_params.application = &g_app_fsref;
-  }
+  if (g_app_fsref)
+    app_params.application = &(g_app_fsref.value());
 
----------------
bulbazord wrote:
> Should we exit early if we don't have `g_app_fsref` filled in? The `application` field of `app_params` won't be filled in so what will `LSOpenURLsWithRole` do?
No, the application is optional. If none is specified we use the default. You can't pass a default constructor one though, it has to be NULL, which is why we have the check here.


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

https://reviews.llvm.org/D149472