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