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

Med Ismail Bennani via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Fri Apr 28 11:12:42 PDT 2023


mib added inline comments.


================
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));
+
----------------
bulbazord wrote:
> Could you document what the NULL and false refer to? I think they're for allocator and isDirectory or something like that.
+1


================
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());
 
----------------
JDevlieghere wrote:
> 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.
Looking at the documentation (https://developer.apple.com/documentation/coreservices/1448184-lsopenurlswithrole), if `app_params.application` is `NULL`, `LSOpenURLsWithRole` makes use of `kLSRolesAll` (https://developer.apple.com/documentation/coreservices/lsrolesmask?language=objc) which I believe open the default application for that file.


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

https://reviews.llvm.org/D149472



More information about the lldb-commits mailing list