Refactor GetNextPersistentVariableName into a non-virtual method

[aprantl]

Summary

Refactor GetNextPersistentVariableName into a non-virtual method
that takes a prefix string. This simplifies the implementation and
allows plugins such as the Swift plugin to supply different prefixes
for return and error variables.

rdar://problem/39722386

[aprantl]

ping

[jingham]

I'm curious how you are going to implement GetPersistentVariablePrefix for a language (e.g. swift) that uses multiple prefixes. You've got a pattern here where you always call GetPersistentVariablePrefix, but you're going to have to break that pattern for swift. And then how are you going to handle that in generic code w/o having to check the language.

Anyway, you shouldn't hardcode $ twice.

source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp

668–670

Was it just an oversight that you don't call m_persistent_state->GetPersistentVariablePrefix here?

[aprantl]

Address review feedback.

[jingham]

That's fine.

include/lldb/Expression/ExpressionVariable.h

public:
   CreatePersistentVariable(const lldb::ValueObjectSP &valobj_sp) = 0;
 
   virtual lldb::ExpressionVariableSP
   CreatePersistentVariable(ExecutionContextScope *exe_scope,
                            const ConstString &name, const CompilerType &type,
                            lldb::ByteOrder byte_order,
                            uint32_t addr_byte_size) = 0;
 
-  virtual ConstString GetNextPersistentVariableName(Target &target) = 0;
+  /// Return a new persistent variable name with the specified prefix.
+  ConstString GetNextPersistentVariableName(Target &target,
+                                            llvm::StringRef prefix);
+
+  virtual llvm::StringRef
+  GetPersistentVariablePrefix(bool is_error = false) const = 0;
 
   virtual void
   RemovePersistentVariable(lldb::ExpressionVariableSP variable) = 0;
 
   virtual lldb::addr_t LookupSymbol(const ConstString &name);
 
   void RegisterExecutionUnit(lldb::IRExecutionUnitSP &execution_unit_sp);
 

source/Core/ValueObject.cpp

ValueObjectSP ValueObject::Persist() {
 
   PersistentExpressionState *persistent_state =
       target_sp->GetPersistentExpressionStateForLanguage(
           GetPreferredDisplayLanguage());
 
   if (!persistent_state)
     return nullptr;
 
-  ConstString name(persistent_state->GetNextPersistentVariableName(*target_sp));
+  auto prefix = persistent_state->GetPersistentVariablePrefix();
+  ConstString name =
+      persistent_state->GetNextPersistentVariableName(*target_sp, prefix);
 
   ValueObjectSP const_result_sp =
       ValueObjectConstResult::Create(target_sp.get(), GetValue(), name);
 
   ExpressionVariableSP clang_var_sp =
       persistent_state->CreatePersistentVariable(const_result_sp);
   clang_var_sp->m_live_sp = clang_var_sp->m_frozen_sp;
   clang_var_sp->m_flags |= ExpressionVariable::EVIsProgramReference;

source/Expression/ExpressionVariable.cpp

 //===-- ExpressionVariable.cpp ----------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "lldb/Expression/ExpressionVariable.h"
 #include "lldb/Expression/IRExecutionUnit.h"
+#include "lldb/Target/Target.h"
 #include "lldb/Utility/Log.h"
 
 using namespace lldb_private;
 
 ExpressionVariable::~ExpressionVariable() {}
 
 uint8_t *ExpressionVariable::GetValueBytes() {
   const size_t byte_size = m_frozen_sp->GetByteSize();
if (global_var.m_remote_addr != LLDB_INVALID_ADDRESS) {
       mangler.GetDemangledName(lldb::eLanguageTypeUnknown);
       m_symbol_map[global_var.m_name.GetCString()] = global_var.m_remote_addr;
       if (log)
         log->Printf("  Symbol: %s at 0x%" PRIx64 ".",
                     global_var.m_name.GetCString(), global_var.m_remote_addr);
     }
   }
 }
+
+ConstString PersistentExpressionState::GetNextPersistentVariableName(
+    Target &target, llvm::StringRef Prefix) {
+  llvm::SmallString<64> name;
+  {
+    llvm::raw_svector_ostream os(name);
+    os << Prefix << target.GetNextPersistentVariableIndex();
+  }
+  return ConstString(name);
+}

source/Expression/Materializer.cpp

if (!persistent_state) {
                          "corresponding type system doesn't handle persistent "
                          "variables");
       return;
     }
 
     ConstString name =
         m_delegate
             ? m_delegate->GetName()
-            : persistent_state->GetNextPersistentVariableName(*target_sp);
+            : persistent_state->GetNextPersistentVariableName(
+                  *target_sp, persistent_state->GetPersistentVariablePrefix());
 
     lldb::ExpressionVariableSP ret = persistent_state->CreatePersistentVariable(
         exe_scope, name, m_type, map.GetByteOrder(), map.GetAddressByteSize());
 
     if (!ret) {
       err.SetErrorStringWithFormat("couldn't dematerialize a result variable: "
                                    "failed to make persistent variable %s",
                                    name.AsCString());

source/Plugins/ExpressionParser/Clang/ClangPersistentVariables.h

public:
   lldb::ExpressionVariableSP
   CreatePersistentVariable(const lldb::ValueObjectSP &valobj_sp) override;
 
   lldb::ExpressionVariableSP CreatePersistentVariable(
       ExecutionContextScope *exe_scope, const ConstString &name,
       const CompilerType &compiler_type, lldb::ByteOrder byte_order,
       uint32_t addr_byte_size) override;
 
-  //----------------------------------------------------------------------
-  /// Return the next entry in the sequence of strings "$0", "$1", ... for
-  /// use naming persistent expression convenience variables.
-  ///
-  /// @return
-  ///     A string that contains the next persistent variable name.
-  //----------------------------------------------------------------------
-  ConstString GetNextPersistentVariableName(Target &target) override;
-
   void RemovePersistentVariable(lldb::ExpressionVariableSP variable) override;
+  llvm::StringRef
+  GetPersistentVariablePrefix(bool is_error) const override {
+    return "$";
+  }
 
   void RegisterPersistentDecl(const ConstString &name, clang::NamedDecl *decl);
 
   clang::NamedDecl *GetPersistentDecl(const ConstString &name);
 
   void AddHandLoadedClangModule(ClangModulesDeclVendor::ModuleID module) {
     m_hand_loaded_clang_modules.push_back(module);
   }

source/Plugins/ExpressionParser/Clang/ClangPersistentVariables.cpp

void ClangPersistentVariables::RemovePersistentVariable(
   if (*name != '$')
     return;
   name++;
 
   if (strtoul(name, NULL, 0) == m_next_persistent_variable_id - 1)
     m_next_persistent_variable_id--;
 }
 
-ConstString
-ClangPersistentVariables::GetNextPersistentVariableName(Target &target) {
-  char name_cstr[256];
-  ::snprintf(name_cstr, sizeof(name_cstr), "$%u",
-             target.GetNextPersistentVariableIndex());
-  ConstString name(name_cstr);
-  return name;
-}
-
 void ClangPersistentVariables::RegisterPersistentDecl(const ConstString &name,
                                                       clang::NamedDecl *decl) {
   m_persistent_decls.insert(
       std::pair<const char *, clang::NamedDecl *>(name.GetCString(), decl));
 
   if (clang::EnumDecl *enum_decl = llvm::dyn_cast<clang::EnumDecl>(decl)) {
     for (clang::EnumConstantDecl *enumerator_decl : enum_decl->enumerators()) {
       m_persistent_decls.insert(std::pair<const char *, clang::NamedDecl *>(

source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp

 
 void ClangUserExpression::ClangUserExpressionHelper::CommitPersistentDecls() {
   if (m_result_synthesizer_up.get()) {
     m_result_synthesizer_up->CommitPersistentDecls();
   }
 }
 
 ConstString ClangUserExpression::ResultDelegate::GetName() {
-  return m_persistent_state->GetNextPersistentVariableName(*m_target_sp);
+  auto prefix = m_persistent_state->GetPersistentVariablePrefix();
+  return m_persistent_state->GetNextPersistentVariableName(*m_target_sp,
+                                                           prefix);
 }
 
 void ClangUserExpression::ResultDelegate::DidDematerialize(
     lldb::ExpressionVariableSP &variable) {
   m_variable = variable;
 }
 
 void ClangUserExpression::ResultDelegate::RegisterPersistentState(
     PersistentExpressionState *persistent_state) {
   m_persistent_state = persistent_state;
 }
 
 lldb::ExpressionVariableSP &ClangUserExpression::ResultDelegate::GetVariable() {
   return m_variable;
 }

source/Plugins/ExpressionParser/Go/GoUserExpression.h

 
 namespace lldb_private {
 class GoParser;
 
 class GoPersistentExpressionState : public PersistentExpressionState {
 public:
   GoPersistentExpressionState();
 
-  ConstString GetNextPersistentVariableName(Target &target) override;
-
+  llvm::StringRef
+  GetPersistentVariablePrefix(bool is_error) const override {
+    return "$go";
+  }
   void RemovePersistentVariable(lldb::ExpressionVariableSP variable) override;
 
   lldb::addr_t LookupSymbol(const ConstString &name) override {
     return LLDB_INVALID_ADDRESS;
   }
 
   static bool classof(const PersistentExpressionState *pv) {
     return pv->getKind() == PersistentExpressionState::eKindGo;

source/Plugins/ExpressionParser/Go/GoUserExpression.cpp

if (!result_val_sp) {
     return lldb::eExpressionDiscarded;
   }
   result.reset(new ExpressionVariable(ExpressionVariable::eKindGo));
   result->m_live_sp = result->m_frozen_sp = result_val_sp;
   result->m_flags |= ExpressionVariable::EVIsProgramReference;
   PersistentExpressionState *pv =
       target->GetPersistentExpressionStateForLanguage(eLanguageTypeGo);
   if (pv != nullptr) {
-    result->SetName(pv->GetNextPersistentVariableName(*target));
+    result->SetName(pv->GetNextPersistentVariableName(
+        *target, pv->GetPersistentVariablePrefix()));
     pv->AddVariable(result);
   }
   return lldb::eExpressionCompleted;
 }
 
 bool GoUserExpression::GoInterpreter::Parse() {
   for (std::unique_ptr<GoASTStmt> stmt(m_parser.Statement()); stmt;
        stmt.reset(m_parser.Statement())) {
if (!value)
     return nullptr;
   // TODO: Handle special conversions
   return value->Cast(type);
 }
 
 GoPersistentExpressionState::GoPersistentExpressionState()
     : PersistentExpressionState(eKindGo) {}
 
-ConstString
-GoPersistentExpressionState::GetNextPersistentVariableName(Target &target) {
-  char name_cstr[256];
-  // We can't use the same variable format as clang.
-  ::snprintf(name_cstr, sizeof(name_cstr), "$go%u",
-             target.GetNextPersistentVariableIndex());
-  ConstString name(name_cstr);
-  return name;
-}
-
 void GoPersistentExpressionState::RemovePersistentVariable(
     lldb::ExpressionVariableSP variable) {
   RemoveVariable(variable);
 
   const char *name = variable->GetName().AsCString();
 
   if (*(name++) != '$')
     return;
   if (*(name++) != 'g')
     return;
   if (*(name++) != 'o')
     return;
 
   if (strtoul(name, nullptr, 0) == m_next_persistent_variable_id - 1)
     m_next_persistent_variable_id--;
 }

source/Target/ABI.cpp

if (persistent) {
     Target &target = *thread.CalculateTarget();
     PersistentExpressionState *persistent_expression_state =
         target.GetPersistentExpressionStateForLanguage(
             ast_type.GetMinimumLanguage());
 
     if (!persistent_expression_state)
       return ValueObjectSP();
 
-    ConstString persistent_variable_name(
-        persistent_expression_state->GetNextPersistentVariableName(target));
+    auto prefix = persistent_expression_state->GetPersistentVariablePrefix();
+    ConstString persistent_variable_name =
+        persistent_expression_state->GetNextPersistentVariableName(target,
+                                                                   prefix);
 
     lldb::ValueObjectSP const_valobj_sp;
 
     // Check in case our value is already a constant value
     if (return_valobj_sp->GetIsConstant()) {
       const_valobj_sp = return_valobj_sp;
       const_valobj_sp->SetName(persistent_variable_name);
     } else