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