JDevlieghere marked 6 inline comments as done. JDevlieghere added a comment.
In D159164#4632017 <https://reviews.llvm.org/D159164#4632017>, @clayborg wrote: > We shouldn't need to pass in "bool use_color" to the Disassembler creation > functions as the only reason to pass something to the creation function for > any plug-in is if that data would exclude a disassembler if it didn't support > color. But we always want to see some disassembly rather than none if a > plug-in is the only one that handles a certain architecture and doesn't > support color. We should either add accessors to enable colorization to the > Disassembler virtual plug-in interface _or_ add "bool use_color" to the > needed functions. Looks DisassembleBytes added a "bool use_color" argument to > the call already, so we shouldn't need a "bool use_color" for the plug-in > selection. Are there other places that need to know about the "use_color" > setting that do not results from a direct function call that can have an > argument added to it? Thanks for the suggestion, that's definitely much nicer. I made it part of the create function because I thought we needed to know the value during initialization time, but you're correct that we can set this after the fact with the help of an accessor. This simplifies the patch by a lot. > Even if colorization is enabled, It would still expect any APIs that return > information in individual instructions, like: > > const char *SBInstruction::GetMnemonic(lldb::SBTarget target); > const char *SBInstruction::GetOperands(lldb::SBTarget target); > const char *SBInstruction::GetComment(lldb::SBTarget target); > > To not have colorization attached to the returned string. Yeah, the updated patch accounts for that and adds a test. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D159164/new/ https://reviews.llvm.org/D159164 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
