jingham added a comment.
This seems fine in general, with one quibble:
IIUC, the "indirect" removal of persistent results which you are trying to
avoid happens here:
lldb::ExpressionResults
UserExpression::Execute(DiagnosticManager &diagnostic_manager,
ExecutionContext &exe_ctx,
const EvaluateExpressionOptions &options,
lldb::UserExpressionSP &shared_ptr_to_me,
lldb::ExpressionVariableSP &result_var) {
lldb::ExpressionResults expr_result = DoExecute(
diagnostic_manager, exe_ctx, options, shared_ptr_to_me, result_var);
Target *target = exe_ctx.GetTargetPtr();
if (options.GetSuppressPersistentResult() && result_var && target) {
if (auto *persistent_state =
target->GetPersistentExpressionStateForLanguage(m_language))
persistent_state->RemovePersistentVariable(result_var);
}
return expr_result;
}
So to succeed in preserving the persistent result, your patch relies on
SuppressPersistentResult being false in the EvaluateExpressionOptions you pass
to the expression. However, you derive the expression options from the command
options, for instance in:
const EvaluateExpressionOptions eval_options =
m_command_options.GetEvaluateExpressionOptions(target, m_varobj_options);
so you don't actually know what the value of the SuppressPersistentResults is
going to be. Since you rely on this having a particular value regardless of
the user's intentions, you should do SetSuppressPersistentResults explicitly
(with an appropriate comment) after you've fetch the eval_options in the
`dwim-print` and `expr` commands.
A much smaller quibble is that it seems a little weird to ask the expression
options or the frame which language in the
PersistentExpressionResultsForLanguage map the result variable was stuffed into
when you have the Result ValueObject on hand. That seems like something the
ValueObject should tell you. When we Persist ValueObjects we use
ValueObject::GetPreferredDisplayLanguage. That seems more trustworthy - if it
isn't we should make it so...
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D150619/new/
https://reviews.llvm.org/D150619
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
