Eng/pr 136978398 #85698
Eng/pr 136978398 #85698
Conversation
There was a problem hiding this comment.
I think we should make BacktraceJSONFormatter a little bit more like BacktraceFormatter, API-wise; I don't think it should be a protocol, with users of the type expected to implement methods from that protocol — rather, I think a lot of those functions should really be handled via a BacktraceJSONFormatterOptions struct.
Also, this PR needs a proper title :-) But you knew that already.
| // Provides functionality to format JSON backtraces. | ||
| // | ||
| //===----------------------------------------------------------------------===// | ||
| import Swift |
There was a problem hiding this comment.
Nit: Let's have a blank line above this:
| import Swift | |
| import Swift |
| static func getDescription() -> String? | ||
| static func getArchitecture() -> String? | ||
| static func getPlatform() -> String? | ||
| static func getFaultAddress() -> String? | ||
| static func allThreads() -> [ThreadType] | ||
| static func crashingThreadIndex() -> Int? | ||
| static func writeThreadRegisters(thread: ThreadType, capturedBytes: inout [UInt64:Array<UInt8>]) | ||
| static func getImages() -> ImageMap? | ||
|
|
||
| // settings for the output format | ||
| static func getShouldShowAllThreads() -> Bool | ||
| static func getShouldShowAllRegisters() -> Bool | ||
| static func getShouldDemangle() -> Bool | ||
| static func getShouldSanitize() -> Bool | ||
| static func getShowMentionedImages() -> Bool |
There was a problem hiding this comment.
IMO most of these should be options in an option type, see e.g. the BacktraceFormattingOptions type in BacktraceFormatter.swift.
It's not obvious from its name what writeThreadRegisters is supposed to do, or why it's a part of this protocol. Maybe that will become clearer as I read through the code below.
| mentionedImages: inout Set<Int>, | ||
| capturedBytes: inout [UInt64:Array<UInt8>]) { |
There was a problem hiding this comment.
Why are these inout? We don't modify them, do we?
There was a problem hiding this comment.
Hmmm. We do modify them, because BacktraceJSONFormatter is a protocol rather than being a class or struct, so there's no state.
I think BacktraceJSONFormatter should be a class or struct (I'll leave which up to you — it doesn't matter too much unless we're going to pass them around), and the mentionedImages and capturedBytes variables should be member variables. The constructor for BacktraceJSONFormatter should take a BacktraceJSONFormatterOptions argument, and it should have a format(backtrace: Backtrace) method and a format(backtrace: SymbolicatedBacktrace) method.
2 participants
Refactoring the JSON writer in
swift-backtraceto allow its reuse. This will be used by the new swift-symbolicate tool.Currently this is in draft for feedback and is not yet complete. The shared JSON formatting code has been moved into the Runtime module.