JDevlieghere created this revision.
JDevlieghere added reviewers: aprantl, bulbazord, mib.
Herald added a project: All.
JDevlieghere requested review of this revision.
This patch refactors the macOS implementation of `OpenFileInExternalEditor`:
- Fix `AppleEvent` memory leak
- Fix caching of `LLDB_EXTERNAL_EDITOR`
- Fix (speculatively) crash when CFURL is NULL.
- Improve readability
- Improve documentation
- Improve logging
rdar://108633464
https://reviews.llvm.org/D149472
Files:
lldb/source/Host/macosx/objcxx/Host.mm
Index: lldb/source/Host/macosx/objcxx/Host.mm
===================================================================
--- lldb/source/Host/macosx/objcxx/Host.mm
+++ lldb/source/Host/macosx/objcxx/Host.mm
@@ -330,7 +330,23 @@
#if !TARGET_OS_OSX
return false;
#else // !TARGET_OS_OSX
- // We attach this to an 'odoc' event to specify a particular selection
+ Log *log = GetLog(LLDBLog::Host);
+
+ const std::string file_path = file_spec.GetPath();
+
+ LLDB_LOG(log, "Sending {0}:{1} to external editor", file_path, line_no);
+
+ if (file_path.empty())
+ return false;
+
+ CFCString file_cfstr(file_path.c_str(), kCFStringEncodingUTF8);
+ CFCReleaser<CFURLRef> file_URL(::CFURLCreateWithFileSystemPath(
+ NULL, file_cfstr.get(), kCFURLPOSIXPathStyle, false));
+
+ if (!file_URL.get())
+ return false;
+
+ // Create a new Apple Event descriptor.
typedef struct {
int16_t reserved0; // must be zero
int16_t fLineNumber;
@@ -340,18 +356,7 @@
uint32_t reserved2; // must be zero
} BabelAESelInfo;
- Log *log = GetLog(LLDBLog::Host);
- char file_path[PATH_MAX];
- file_spec.GetPath(file_path, PATH_MAX);
- CFCString file_cfstr(file_path, kCFStringEncodingUTF8);
- CFCReleaser<CFURLRef> file_URL(::CFURLCreateWithFileSystemPath(
- NULL, file_cfstr.get(), kCFURLPOSIXPathStyle, false));
-
- LLDB_LOGF(log,
- "Sending source file: \"%s\" and line: %d to external editor.\n",
- file_path, line_no);
-
- long error;
+ // We attach this to an 'odoc' event to specify a particular selection.
BabelAESelInfo file_and_line_info = {
0, // reserved0
(int16_t)(line_no - 1), // fLineNumber (zero based line number)
@@ -362,60 +367,62 @@
};
AEKeyDesc file_and_line_desc;
-
- error = ::AECreateDesc(typeUTF8Text, &file_and_line_info,
- sizeof(file_and_line_info),
- &(file_and_line_desc.descContent));
+ file_and_line_desc.descKey = keyAEPosition;
+ long error = ::AECreateDesc(/*typeCode=*/typeUTF8Text,
+ /*dataPtr=*/&file_and_line_info,
+ /*dataSize=*/sizeof(file_and_line_info),
+ /*result=*/&(file_and_line_desc.descContent));
if (error != noErr) {
- LLDB_LOGF(log, "Error creating AEDesc: %ld.\n", error);
+ LLDB_LOG(log, "Creating Apple Event descriptor failed: error {0}", error);
return false;
}
- file_and_line_desc.descKey = keyAEPosition;
+ // Deallocate the descriptor on exit.
+ auto on_exit = llvm::make_scope_exit(
+ [&]() { AEDisposeDesc(&(file_and_line_desc.descContent)); });
- static std::string g_app_name;
- static FSRef g_app_fsref;
+ static std::optional<FSRef> g_app_fsref;
+ 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);
+ FSRef app_fsref;
+ CFCString editor_name(external_editor, kCFStringEncodingUTF8);
+ error = ::LSFindApplicationForInfo(
+ /*inCreator=*/kLSUnknownCreator, /*inBundleID=*/NULL,
+ /*inName=*/editor_name.get(), /*outAppRef=*/&app_fsref,
+ /*outAppURL=*/NULL);
+ if (error == noErr) {
+ g_app_fsref = app_fsref;
+ } else {
+ LLDB_LOG(log, "Could not find external editor \"{0}\": error {1}",
+ external_editor, error);
+ }
+ }
+ });
+
+ // Build app launch parameters.
LSApplicationParameters app_params;
::memset(&app_params, 0, sizeof(app_params));
app_params.flags =
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());
ProcessSerialNumber psn;
- CFCReleaser<CFArrayRef> file_array(
- CFArrayCreate(NULL, (const void **)file_URL.ptr_address(false), 1, NULL));
- error = ::LSOpenURLsWithRole(file_array.get(), kLSRolesAll,
- &file_and_line_desc, &app_params, &psn, 1);
-
- AEDisposeDesc(&(file_and_line_desc.descContent));
+ std::array<CFURLRef, 1> file_array = {file_URL.get()};
+ CFCReleaser<CFArrayRef> cf_array(
+ CFArrayCreate(/*allocator=*/NULL, /*values=*/(const void **)&file_array,
+ /*numValues*/ 1, /*callBacks=*/NULL));
+ error = ::LSOpenURLsWithRole(
+ /*inURLs=*/cf_array.get(), /*inRole=*/kLSRolesAll,
+ /*inAEParam=*/&file_and_line_desc,
+ /*inAppParams=*/&app_params, /*outPSNs=*/&psn, /*inMaxPSNCount=*/1);
if (error != noErr) {
- LLDB_LOGF(log, "LSOpenURLsWithRole failed, error: %ld.\n", error);
-
+ LLDB_LOG(log, "LSOpenURLsWithRole failed: error {0}", error);
return false;
}
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
