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 +DisassemblerLLVMC::MCDisasmToolset::MCDisasmToolset( + 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, ---------------- clayborg wrote: > 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). https://reviews.llvm.org/D41584 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
