Address/fix remaining reviewer comments:

cmtice · cmtice · commit 42c52f3146fa · 2025-03-16T10:40:17.000-07:00
- Rename 'NewFormatDiagnostics'	to 'FormatDiagnostics',	keeping	the code that
  uses DiagnosticDetail; remove	'OldFormatDiagnostics'.
- Remove the code that automatically dereferences reference variables before
  returning them.
- Remove IdentifierInfo	class, replacing it directly with lldb::ValueObjectSP
  values.
diff --git a/lldb/include/lldb/ValueObject/DILEval.h b/lldb/include/lldb/ValueObject/DILEval.h
@@ -16,54 +16,25 @@
 
 namespace lldb_private::dil {
 
-/// Class used to store & manipulate information about identifiers.
-class IdentifierInfo {
-public:
-  enum class Kind {
-    eValue,
-    eContextArg,
-  };
-
-  static std::unique_ptr<IdentifierInfo> FromValue(ValueObject &valobj) {
-    CompilerType type;
-    type = valobj.GetCompilerType();
-    return std::unique_ptr<IdentifierInfo>(
-        new IdentifierInfo(Kind::eValue, type, valobj.GetSP(), {}));
-  }
-
-  Kind GetKind() const { return m_kind; }
-  lldb::ValueObjectSP GetValue() const { return m_value; }
-
-  bool IsValid() const { return m_type.IsValid(); }
-
-  IdentifierInfo(Kind kind, CompilerType type, lldb::ValueObjectSP value,
-                 std::vector<uint32_t> path)
-      : m_kind(kind), m_type(type), m_value(std::move(value)) {}
-
-private:
-  Kind m_kind;
-  CompilerType m_type;
-  lldb::ValueObjectSP m_value;
-};
-
 /// Given the name of an identifier (variable name, member name, type name,
 /// etc.), find the ValueObject for that name (if it exists), excluding global
 /// variables, and create and return an IdentifierInfo object containing all
 /// the relevant information about that object (for DIL parsing and
 /// evaluating).
-std::unique_ptr<IdentifierInfo>
-LookupIdentifier(llvm::StringRef name_ref, std::shared_ptr<StackFrame> frame_sp,
-                 lldb::DynamicValueType use_dynamic,
-                 CompilerType *scope_ptr = nullptr);
+lldb::ValueObjectSP LookupIdentifier(llvm::StringRef name_ref,
+                                     std::shared_ptr<StackFrame> frame_sp,
+                                     lldb::DynamicValueType use_dynamic,
+                                     CompilerType *scope_ptr = nullptr);
 
 /// Given the name of an identifier, check to see if it matches the name of a
 /// global variable. If so, find the ValueObject for that global variable, and
 /// create and return an IdentifierInfo object containing all the relevant
 /// informatin about it.
-std::unique_ptr<IdentifierInfo> LookupGlobalIdentifier(
-    llvm::StringRef name_ref, std::shared_ptr<StackFrame> frame_sp,
-    lldb::TargetSP target_sp, lldb::DynamicValueType use_dynamic,
-    CompilerType *scope_ptr = nullptr);
+lldb::ValueObjectSP LookupGlobalIdentifier(llvm::StringRef name_ref,
+                                           std::shared_ptr<StackFrame> frame_sp,
+                                           lldb::TargetSP target_sp,
+                                           lldb::DynamicValueType use_dynamic,
+                                           CompilerType *scope_ptr = nullptr);
 
 class Interpreter : Visitor {
 public:
@@ -77,16 +48,11 @@ class Interpreter : Visitor {
   llvm::Expected<lldb::ValueObjectSP>
   Visit(const IdentifierNode *node) override;
 
-private:
   // Used by the interpreter to create objects, perform casts, etc.
   lldb::TargetSP m_target;
-
   llvm::StringRef m_expr;
-
   lldb::ValueObjectSP m_scope;
-
   lldb::DynamicValueType m_default_dynamic;
-
   std::shared_ptr<StackFrame> m_exe_ctx_scope;
 };
 
diff --git a/lldb/include/lldb/ValueObject/DILParser.h b/lldb/include/lldb/ValueObject/DILParser.h
@@ -28,12 +28,9 @@ enum class ErrorCode : unsigned char {
   kUnknown,
 };
 
-std::string NewFormatDiagnostics(llvm::StringRef input_expr,
-                                 const std::string &message, uint32_t loc,
-                                 uint16_t err_len);
-
-std::string OldFormatDiagnostics(llvm::StringRef input_expr,
-                                 const std::string &message, uint32_t loc);
+std::string FormatDiagnostics(llvm::StringRef input_expr,
+                              const std::string &message, uint32_t loc,
+                              uint16_t err_len);
 
 /// Pure recursive descent parser for C++ like expressions.
 /// EBNF grammar for the parser is described in lldb/docs/dil-expr-lang.ebnf
diff --git a/lldb/source/ValueObject/DILEval.cpp b/lldb/source/ValueObject/DILEval.cpp
@@ -87,7 +87,7 @@ static lldb::VariableSP DILFindVariable(ConstString name,
   return nullptr;
 }
 
-std::unique_ptr<IdentifierInfo> LookupGlobalIdentifier(
+lldb::ValueObjectSP LookupGlobalIdentifier(
     llvm::StringRef name_ref, std::shared_ptr<StackFrame> stack_frame,
     lldb::TargetSP target_sp, lldb::DynamicValueType use_dynamic,
     CompilerType *scope_ptr) {
@@ -105,7 +105,7 @@ std::unique_ptr<IdentifierInfo> LookupGlobalIdentifier(
   }
 
   if (value_sp)
-    return IdentifierInfo::FromValue(*value_sp);
+    return value_sp;
 
   // Also check for static global vars.
   if (variable_list) {
@@ -122,7 +122,7 @@ std::unique_ptr<IdentifierInfo> LookupGlobalIdentifier(
   }
 
   if (value_sp)
-    return IdentifierInfo::FromValue(*value_sp);
+    return value_sp;
 
   // Check for match in modules global variables.
   VariableList modules_var_list;
@@ -142,15 +142,15 @@ std::unique_ptr<IdentifierInfo> LookupGlobalIdentifier(
   }
 
   if (value_sp)
-    return IdentifierInfo::FromValue(*value_sp);
+    return value_sp;
 
   return nullptr;
 }
 
-std::unique_ptr<IdentifierInfo>
-LookupIdentifier(llvm::StringRef name_ref,
-                 std::shared_ptr<StackFrame> stack_frame,
-                 lldb::DynamicValueType use_dynamic, CompilerType *scope_ptr) {
+lldb::ValueObjectSP LookupIdentifier(llvm::StringRef name_ref,
+                                     std::shared_ptr<StackFrame> stack_frame,
+                                     lldb::DynamicValueType use_dynamic,
+                                     CompilerType *scope_ptr) {
   // Support $rax as a special syntax for accessing registers.
   // Will return an invalid value in case the requested register doesn't exist.
   if (name_ref.consume_front("$")) {
@@ -165,7 +165,7 @@ LookupIdentifier(llvm::StringRef name_ref,
           ValueObjectRegister::Create(stack_frame.get(), reg_ctx, reg_info);
 
     if (value_sp)
-      return IdentifierInfo::FromValue(*value_sp);
+      return value_sp;
 
     return nullptr;
   }
@@ -189,7 +189,7 @@ LookupIdentifier(llvm::StringRef name_ref,
         value_sp = stack_frame->FindVariable(ConstString(name_ref));
 
       if (value_sp)
-        return IdentifierInfo::FromValue(*value_sp);
+        return value_sp;
 
       // Try looking for an instance variable (class member).
       SymbolContext sc = stack_frame->GetSymbolContext(
@@ -200,7 +200,7 @@ LookupIdentifier(llvm::StringRef name_ref,
         value_sp = value_sp->GetChildMemberWithName(name_ref);
 
       if (value_sp)
-        return IdentifierInfo::FromValue(*(value_sp));
+        return value_sp;
     }
   }
   return nullptr;
@@ -226,7 +226,7 @@ llvm::Expected<lldb::ValueObjectSP>
 Interpreter::Visit(const IdentifierNode *node) {
   lldb::DynamicValueType use_dynamic = node->GetUseDynamic();
 
-  std::unique_ptr<IdentifierInfo> identifier =
+  lldb::ValueObjectSP identifier =
       LookupIdentifier(node->GetName(), m_exe_ctx_scope, use_dynamic);
 
   if (!identifier)
@@ -235,28 +235,14 @@ Interpreter::Visit(const IdentifierNode *node) {
   if (!identifier) {
     std::string errMsg =
         llvm::formatv("use of undeclared identifier '{0}'", node->GetName());
-    Status error = Status(
-        (uint32_t)ErrorCode::kUndeclaredIdentifier, lldb::eErrorTypeGeneric,
-        NewFormatDiagnostics(m_expr, errMsg, node->GetLocation(),
-                             node->GetName().size()));
+    Status error = Status((uint32_t)ErrorCode::kUndeclaredIdentifier,
+                          lldb::eErrorTypeGeneric,
+                          FormatDiagnostics(m_expr, errMsg, node->GetLocation(),
+                                            node->GetName().size()));
     return error.ToError();
   }
-  lldb::ValueObjectSP val;
-  lldb::TargetSP target_sp;
 
-  assert(identifier->GetKind() == IdentifierInfo::Kind::eValue &&
-         "Unrecognized identifier kind");
-
-  val = identifier->GetValue();
-
-  if (val->GetCompilerType().IsReferenceType()) {
-    Status error;
-    val = val->Dereference(error);
-    if (error.Fail())
-      return error.ToError();
-  }
-
-  return val;
+  return identifier;
 }
 
 } // namespace lldb_private::dil
diff --git a/lldb/source/ValueObject/DILParser.cpp b/lldb/source/ValueObject/DILParser.cpp
@@ -26,9 +26,8 @@
 
 namespace lldb_private::dil {
 
-std::string NewFormatDiagnostics(llvm::StringRef text,
-                                 const std::string &message, uint32_t loc,
-                                 uint16_t err_len) {
+std::string FormatDiagnostics(llvm::StringRef text, const std::string &message,
+                              uint32_t loc, uint16_t err_len) {
   DiagnosticDetail::SourceLocation sloc = {
       FileSpec{}, /*line=*/1, loc + 1, err_len, false, /*in_user_input=*/true};
   std::string arrow_str = "^";
@@ -48,18 +47,6 @@ std::string NewFormatDiagnostics(llvm::StringRef text,
   return ret_str;
 }
 
-std::string OldFormatDiagnostics(llvm::StringRef text,
-                                 const std::string &message, uint32_t loc) {
-  // Get the position, in the current line of text, of the diagnostics pointer.
-  // ('loc' is the location of the start of the current token/error within the
-  // overall text line).
-  int32_t arrow = loc + 1; // Column offset starts at 1, not 0.
-
-  return llvm::formatv("<expr:1:{0}>: {1}\n{2}\n{3}", loc + 1, message,
-                       llvm::fmt_pad(text, 0, 0),
-                       llvm::fmt_pad("^", arrow - 1, 0));
-}
-
 llvm::Expected<ASTNodeUP>
 DILParser::Parse(llvm::StringRef dil_input_expr, DILLexer lexer,
                  std::shared_ptr<StackFrame> frame_sp,
@@ -257,8 +244,8 @@ void DILParser::BailOut(ErrorCode code, const std::string &error,
   }
 
   m_error = Status((uint32_t)code, lldb::eErrorTypeGeneric,
-                   NewFormatDiagnostics(m_input_expr, error, loc,
-                                        CurToken().GetSpelling().length()));
+                   FormatDiagnostics(m_input_expr, error, loc,
+                                     CurToken().GetSpelling().length()));
   // Advance the lexer token index to the end of the lexed tokens vector.
   m_dil_lexer.ResetTokenIdx(m_dil_lexer.NumLexedTokens() - 1);
 }
diff --git a/lldb/test/API/commands/frame/var-dil/basics/GlobalVariableLookup/TestFrameVarDILGlobalVariableLookup.py b/lldb/test/API/commands/frame/var-dil/basics/GlobalVariableLookup/TestFrameVarDILGlobalVariableLookup.py
@@ -26,10 +26,10 @@ def test_frame_var(self):
         self.expect("settings set target.experimental.use-DIL true", substrs=[""])
         self.expect_var_path("globalVar", type="int", value="-559038737")  # 0xDEADBEEF
         self.expect_var_path("globalPtr", type="int *")
-        self.expect_var_path("globalRef", value="-559038737")
+        self.expect_var_path("globalRef", type="int &")
         self.expect_var_path("::globalVar", value="-559038737")
         self.expect_var_path("::globalPtr", type="int *")
-        self.expect_var_path("::globalRef", value="-559038737")
+        self.expect_var_path("::globalRef", type="int &")
 
         self.expect(
             "frame variable 'externGlobalVar'",
@@ -44,6 +44,7 @@ def test_frame_var(self):
 
         self.expect_var_path("ns::globalVar", value="13")
         self.expect_var_path("ns::globalPtr", type="int *")
-        self.expect_var_path("ns::globalRef", value="13")
+        self.expect_var_path("ns::globalRef", type="int &")
         self.expect_var_path("::ns::globalVar", value="13")
         self.expect_var_path("::ns::globalPtr", type="int *")
+        self.expect_var_path("::ns::globalRef", type="int &")
