[Lldb-commits] [PATCH] D41584: Check existence of each required component during construction of LLVMCDisassembler.

[Pavel Labath via Phabricator via lldb-commits]

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> &&reg_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