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

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

LGTM overall. A few nits but overall an excellent cleanup! :)



================
Comment at: lldb/source/Host/macosx/objcxx/Host.mm:337
+
+  LLDB_LOG(log, "Sending {0}:{1} to external editor", file_path, line_no);
+
----------------
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.


================
Comment at: lldb/source/Host/macosx/objcxx/Host.mm:343-344
+  CFCString file_cfstr(file_path.c_str(), kCFStringEncodingUTF8);
+  CFCReleaser<CFURLRef> file_URL(::CFURLCreateWithFileSystemPath(
+      NULL, file_cfstr.get(), kCFURLPOSIXPathStyle, false));
+
----------------
Could you document what the NULL and false refer to? I think they're for allocator and isDirectory or something like that.


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


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


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

https://reviews.llvm.org/D149472