[Lldb-commits] [PATCH] D150619: [lldb] Delay removal of persistent results

[Jim Ingham via Phabricator via lldb-commits]

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