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

_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to