[Lldb-commits] [PATCH] D41584: Check existence of each required component during construction of LLVMCDisassembler.
Pavel Labath via Phabricator via lldb-commits
lldb-commits at lists.llvm.org
Wed Jan 10 02:37:41 PST 2018
labath accepted this revision.
labath added a comment.
Looks good to me. Just make sure to respond to all of Greg's comments as well.
Comment at: source/Plugins/Disassembler/llvm/DisassemblerLLVMC.cpp:989-1003
+ std::unique_ptr<llvm::MCInstrInfo> &&instr_info,
+ std::unique_ptr<llvm::MCRegisterInfo> &®_info,
+ std::unique_ptr<llvm::MCSubtargetInfo> &&subtarget_info,
+ std::unique_ptr<llvm::MCAsmInfo> &&asm_info,
+ std::unique_ptr<llvm::MCContext> &&context,
+ std::unique_ptr<llvm::MCDisassembler> &&disasm,
> Do we need this? We are placing the MCDisasmToolset into std::unique_ptr<> member variables so this shouldn't be needed by anyone as the std::unique_ptr already has a move operator
The `std::move` is necessary in this context. The rvalue references can be dropped theoretically -- that will enforce that the constructor takes ownership of the passed-in arguments (as they will get implicitly deleted otherwise) -- right now the constructor can just do nothing and the caller will keep owning these objects.
However, this distinction seldom matters.
Comment at: source/Plugins/Disassembler/llvm/DisassemblerLLVMC.cpp:1002
+ assert(m_instr_info && m_reg_info && m_subtarget_info && m_asm_info &&
+ m_context && disasm && instr_printer);
this should be `m_disasm` and `m_instr_printer` (and make sure to test this with assertions enabled).
More information about the lldb-commits