[Lldb-commits] [PATCH] D41584: Check existence of each required component during construction of LLVMCDisassembler.
Jason Molenda via lldb-commits
lldb-commits at lists.llvm.org
Tue Jan 9 12:05:02 PST 2018
> On Jan 9, 2018, at 6:24 AM, Pavel Labath via Phabricator <reviews at reviews.llvm.org> wrote:
> labath added a comment.
> In https://reviews.llvm.org/D41584#970945, @tatyana-krasnukha wrote:
>> Thank you, Pavel.
>> Would you mind if I move LLVMCDisassembler declaration in .cpp also? It looks like perfect candidate for pimpl.
> Yes, that sounds like a good idea.
>> And... doesn't DisassemblerLLVMC::LLVMCDisassembler confuse anyone but me?)
> Well... I haven't looked at this class in the past, but yes... it looks confusing. I wouldn't mind a name change.
> Jason, any thoughts on this?
I think the issue is in DisassemblerLLVMC.cpp/.h we have a DisassemblerLLVMC class which implements the Disassembler protocol in lldb and providing the usual Plugin methods (Initialize, Terminate, CreateInstance).
The DisassemblerLLVMC class defines a class itself, LLVMCDisassembler, which is the disassembler (so we can have multiple of them for arm/thumb situations). I don't think we can hoist the LLVMCDisassembler methods up into DisassemblerLLVMC because we need more than one. And I'm not great at c++ language lawyer stuff but I don't think we can have a DisassemblerLLVMC class that defines a DisassemblerLLVMC class inside itself.
I'm cool with looking at different names than LLVMCDisassembler, it does look like a mistake more than something done on purpose, but I don't think it's as easy as search & replace unless I've misunderstood.
More information about the lldb-commits