jankratochvil marked 3 inline comments as done. jankratochvil added a comment.
In D67589#1670966 <https://reviews.llvm.org/D67589#1670966>, @labath wrote: > I agree that this approach isn't nice, but probably there isn't a nice > approach to this at this point. One possibility you could consider is storing > a shared_ptr<CommandRO> inside SBCommandRO and encoding the ownership into > the deleter of the shared_ptr (regular deleter => owned, noop deleter => > unowned). Maybe also possibly cheaper `llvm::PointerIntPair` representing an associated `bool` for the deletion. ================ Comment at: lldb/packages/Python/lldbsuite/test/api/command-return-object/main.cpp:18 + result = subcommand(dbg, "help"); + result = result; + if (!result.Succeeded()) ---------------- labath wrote: > Is that intentional? If so, why? It has no purpose there for this bugfix but when already writing a testcase I wanted to test this generally fragile functionality. ================ Comment at: lldb/source/API/SBCommandInterpreter.cpp:165-172 + SBCommandReturnObject sb_return; + std::swap(result, SBCommandReturnObject_ref(sb_return)); SBCommandInterpreter sb_interpreter(&m_interpreter); SBDebugger debugger_sb(m_interpreter.GetDebugger().shared_from_this()); bool ret = m_backend->DoExecute( debugger_sb, (char **)command.GetArgumentVector(), sb_return); + std::swap(result, SBCommandReturnObject_ref(sb_return)); ---------------- clayborg wrote: > Could this code just create a local SBCommandReturnObject and then copy the > CommandReturnObject back into "result"? > > ``` > bool DoExecute(Args &command, CommandReturnObject &result) override { > SBCommandReturnObject sb_return; > SBCommandInterpreter sb_interpreter(&m_interpreter); > SBDebugger debugger_sb(m_interpreter.GetDebugger().shared_from_this()); > bool ret = m_backend->DoExecute( > debugger_sb, (char **)command.GetArgumentVector(), sb_return); > std::swap(result, sb_return.ref()); > return ret; > } > ``` I made it that way first but it breaks the testsuite. For example due to: ``` lldb/source/Commands/CommandObjectCommands.cpp: 1236 bool DoExecute(llvm::StringRef raw_command_line, 1237 CommandReturnObject &result) override { ... 1242 result.SetStatus(eReturnStatusInvalid); - ctored CommandReturnObject has eReturnStatusStarted. 1244 if (!scripter || 1245 !scripter->RunScriptBasedCommand(m_function_name.c_str(), 1246 raw_command_line, m_synchro, result, 1247 error, m_exe_ctx)) { 1248 result.AppendError(error.AsCString()); 1249 result.SetStatus(eReturnStatusFailed); 1250 } else { 1251 // Don't change the status if the command already set it... 1252 if (result.GetStatus() == eReturnStatusInvalid) { 1253 if (result.GetOutputData().empty()) 1254 result.SetStatus(eReturnStatusSuccessFinishNoResult); 1255 else 1256 result.SetStatus(eReturnStatusSuccessFinishResult); 1257 } 1258 } ``` So it would be possible but that would require refactorization more of the code in callers which I find outside of the scope of this patch. This patch tries to make this bugfix fully transparent to existing code. Repository: rLLDB LLDB CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67589/new/ https://reviews.llvm.org/D67589 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
