As it exists today, Host::SystemLog is used exclusively for error reporting. With the introduction of diagnostic events, we have a better way of reporting those. By switching over, we print these error messages to the debugger's error stream (when using the default event handler) or report them to an IDE such as Xcode (if they have subscribed to these events). It also means we nog longer write the messages to the system log on Darwin, but as far as I know, nobody is relying on this functionality today. If this is deemed important externally, I can add it again when I add the system logging functionality again in the context of D128321.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
lldb/source/Core/Module.cpp | ||
---|---|---|
1187 | It's unrelated to this patch, but it looks like Debugger::HandleDiagnosticEvent dumps everything to the error stream without checking if the event is a warning or an error. I'm mentioning that here because if we did that distinction at the event handling level, we could get rid of strm.PutCString("error: "). Note however, the reason I'm bringing this up, is that the error message prefix is not consistent with ReportErrorIfModifyDetected, since it's not prepended by error: . We should at least fix that for this patch before before making it consistent at the event handler level. |
lldb/source/Core/Module.cpp | ||
---|---|---|
1187 | I hadn't looked at the rest of the patch and this looks very inconsistent throughout the other files as well ... may be we should just leave that for a follow-up patch |
lldb/source/Core/Module.cpp | ||
---|---|---|
1187 | Yeah this is an oversight. The prefix will get printed when the event gets handled (or by the IDE) so it shouldn't be part of the message. I'll fix that here. |
I don't have anything against this, though I still think the LogHandlers should use the Host layer by making the LogHandler objects into real LLDB plug-ins. But if everyone else feels otherwise, I won't object.
This patch is meant to be entirely orthogonal to that. I was still going to add (what I believe is a better) implementation of SystemLog. In D128321 I said:
But if we really want this to live in Host (and deal with the dependency issues later) I can implement a new Host::SystemLog variant that essentially does what the SystemLogHander is doing and implement the log handler in Host based on that.
So that's what I was alluding to.
It's unrelated to this patch, but it looks like Debugger::HandleDiagnosticEvent dumps everything to the error stream without checking if the event is a warning or an error. I'm mentioning that here because if we did that distinction at the event handling level, we could get rid of strm.PutCString("error: ").
Note however, the reason I'm bringing this up, is that the error message prefix is not consistent with ReportErrorIfModifyDetected, since it's not prepended by error: .
We should at least fix that for this patch before before making it consistent at the event handler level.