[Lldb-commits] [PATCH] D128321: [lldb] Add OSLog log handler

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.

In D128321#3605592 <https://reviews.llvm.org/D128321#3605592>, @JDevlieghere wrote:

> I also considered that, but that just delays the problem until I want to enable this handler for the always-on logging (as outlined in https://discourse.llvm.org/c/subprojects/lldb/8). I haven't really looked into where that logic would live, but the reproducers lived in Utility so I was expecting all of this to be there as well.

I don't think that's a particularly big problem. Any code that's needed to enable always-on logging (to /a/ log handler) can stay in Utility. Only the code which actually enables it (and passes a specific log handler) needs to live outside. One way to do that would be to put something in the SystemInitializer class (like we did for reproducers). Another would be to use the shiny new global lldbinit file feature (which would allow site customization). :P



================
Comment at: lldb/source/Host/common/Host.cpp:110
+#if !defined(__APPLE__)
+void Host::SystemLog(const std::string &message) {
+  fprintf(stderr, "%s", message.c_str());
----------------
Why std::string? I'd expect StringRef (as that's what the LogHandler class uses), const char * (as that can avoid string copying sometimes) or a Twine (in case you want to be fancy), but a string seems like it combines the worst properties of all of these.


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

https://reviews.llvm.org/D128321