Address some review comments:

cmtice · cmtice · commit 5d592e3d3912 · 2025-03-11T21:49:43.000-07:00
- Remove FromContextArg	and GetType methods from IdentifierInfo.
- Update Token::IsOneOf to take an ArrayRef rather than	variadic list.
- Various minor	code cleanups.
- Reviewer sugggested code cleanups in DILFindVariable and
  LookupGlobalIdentifier.
- Use lldbutil.run_to_source_breakpont to greatly simplify the DIL python API
  tests.
- Update the DIL lexer unit tests to use the new IsOneOf token parameter
  format.
diff --git a/lldb/include/lldb/ValueObject/DILEval.h b/lldb/include/lldb/ValueObject/DILEval.h
@@ -31,16 +31,9 @@ class IdentifierInfo {
         new IdentifierInfo(Kind::eValue, type, valobj.GetSP(), {}));
   }
 
-  static std::unique_ptr<IdentifierInfo> FromContextArg(CompilerType type) {
-    lldb::ValueObjectSP empty_value;
-    return std::unique_ptr<IdentifierInfo>(
-        new IdentifierInfo(Kind::eContextArg, type, empty_value, {}));
-  }
-
   Kind GetKind() const { return m_kind; }
   lldb::ValueObjectSP GetValue() const { return m_value; }
 
-  CompilerType GetType() { return m_type; }
   bool IsValid() const { return m_type.IsValid(); }
 
   IdentifierInfo(Kind kind, CompilerType type, lldb::ValueObjectSP value,
diff --git a/lldb/include/lldb/ValueObject/DILLexer.h b/lldb/include/lldb/ValueObject/DILLexer.h
@@ -42,22 +42,8 @@ class Token {
 
   bool IsNot(Kind kind) const { return m_kind != kind; }
 
-  bool IsOneOf(Kind kind1, Kind kind2) const { return Is(kind1) || Is(kind2); }
-
-  bool IsOneOf(std::vector<Kind> kinds) const {
-    if (kinds.empty())
-      return false;
-
-    if (kinds.size() == 1)
-      return Is(kinds[0]);
-
-    Kind k = kinds.back();
-    kinds.pop_back();
-    return Is(k) || IsOneOf(kinds);
-  }
-
-  template <typename... Ts> bool IsOneOf(Kind kind, Ts... Ks) const {
-    return Is(kind) || IsOneOf(Ks...);
+  bool IsOneOf(llvm::ArrayRef<Kind> kinds) const {
+    return llvm::is_contained(kinds, m_kind);
   }
 
   uint32_t GetLocation() const { return m_start_pos; }
diff --git a/lldb/include/lldb/ValueObject/DILParser.h b/lldb/include/lldb/ValueObject/DILParser.h
@@ -42,7 +42,7 @@ class DILParser {
                                          bool use_synthetic, bool fragile_ivar,
                                          bool check_ptr_vs_member);
 
-  ~DILParser() { m_ctx_scope.reset(); }
+  ~DILParser() = default;
 
   bool UseSynthetic() { return m_use_synthetic; }
 
diff --git a/lldb/source/Target/StackFrame.cpp b/lldb/source/Target/StackFrame.cpp
@@ -514,9 +514,7 @@ ValueObjectSP StackFrame::GetValueForVariableExpressionPath(
     VariableSP &var_sp, Status &error) {
   ExecutionContext exe_ctx;
   CalculateExecutionContext(exe_ctx);
-
   bool use_DIL = exe_ctx.GetTargetRef().GetUseDIL(&exe_ctx);
-
   if (use_DIL)
     return DILGetValueForVariableExpressionPath(var_expr, use_dynamic, options,
                                                 var_sp, error);
@@ -542,12 +540,11 @@ ValueObjectSP StackFrame::DILGetValueForVariableExpressionPath(
     error = Status::FromError(lex_or_err.takeError());
     return ValueObjectSP();
   }
-  dil::DILLexer &lexer = *lex_or_err;
 
   // Parse the expression.
   auto tree_or_error = dil::DILParser::Parse(
-      var_expr, lexer, shared_from_this(), use_dynamic, !no_synth_child,
-      !no_fragile_ivar, check_ptr_vs_member);
+      var_expr, std::move(*lex_or_err), shared_from_this(), use_dynamic,
+      !no_synth_child, !no_fragile_ivar, check_ptr_vs_member);
   if (!tree_or_error) {
     error = Status::FromError(tree_or_error.takeError());
     return ValueObjectSP();
diff --git a/lldb/source/ValueObject/DILAST.cpp b/lldb/source/ValueObject/DILAST.cpp
@@ -13,7 +13,6 @@ namespace lldb_private::dil {
 
 llvm::Expected<lldb::ValueObjectSP> ErrorNode::Accept(Visitor *v) const {
   llvm_unreachable("Attempting to Visit a DIL ErrorNode.");
-  return lldb::ValueObjectSP();
 }
 
 llvm::Expected<lldb::ValueObjectSP> IdentifierNode::Accept(Visitor *v) const {
diff --git a/lldb/source/ValueObject/DILEval.cpp b/lldb/source/ValueObject/DILEval.cpp
@@ -52,19 +52,15 @@ static lldb::VariableSP DILFindVariable(ConstString name,
   lldb::VariableSP exact_match;
   std::vector<lldb::VariableSP> possible_matches;
 
-  typedef std::vector<lldb::VariableSP> collection;
-  typedef collection::iterator iterator;
-
-  iterator pos, end = variable_list->end();
-  for (pos = variable_list->begin(); pos != end; ++pos) {
-    llvm::StringRef str_ref_name = pos->get()->GetName().GetStringRef();
+  for (lldb::VariableSP var_sp : *variable_list) {
+    llvm::StringRef str_ref_name = var_sp->GetName().GetStringRef();
     // Check for global vars, which might start with '::'.
     str_ref_name.consume_front("::");
 
     if (str_ref_name == name.GetStringRef())
-      possible_matches.push_back(*pos);
-    else if (pos->get()->NameMatches(name))
-      possible_matches.push_back(*pos);
+      possible_matches.push_back(var_sp);
+    else if (var_sp->NameMatches(name))
+      possible_matches.push_back(var_sp);
   }
 
   // Look for exact matches (favors local vars over global vars)
@@ -74,23 +70,21 @@ static lldb::VariableSP DILFindVariable(ConstString name,
       });
 
   if (exact_match_it != possible_matches.end())
-    exact_match = *exact_match_it;
-
-  if (!exact_match)
-    // Look for a global var exact match.
-    for (auto var_sp : possible_matches) {
-      llvm::StringRef str_ref_name = var_sp->GetName().GetStringRef();
-      str_ref_name.consume_front("::");
-      if (str_ref_name == name.GetStringRef()) {
-        exact_match = var_sp;
-        break;
-      }
-    }
+    return *exact_match_it;
 
-  if (!exact_match && possible_matches.size() == 1)
-    exact_match = possible_matches[0];
+  // Look for a global var exact match.
+  for (auto var_sp : possible_matches) {
+    llvm::StringRef str_ref_name = var_sp->GetName().GetStringRef();
+    str_ref_name.consume_front("::");
+    if (str_ref_name == name.GetStringRef())
+      return var_sp;
+  }
+
+  // If there's a single non-exact match, take it.
+  if (possible_matches.size() == 1)
+    return possible_matches[0];
 
-  return exact_match;
+  return nullptr;
 }
 
 std::unique_ptr<IdentifierInfo> LookupGlobalIdentifier(
@@ -239,17 +233,15 @@ Interpreter::Visit(const IdentifierNode *node) {
     identifier = LookupGlobalIdentifier(node->GetName(), m_exe_ctx_scope,
                                         m_target, use_dynamic);
   if (!identifier) {
-    std::string errMsg;
-    std::string name = node->GetName();
-    errMsg = llvm::formatv("use of undeclared identifier '{0}'", name);
+    std::string errMsg =
+        llvm::formatv("use of undeclared identifier '{0}'", node->GetName());
     Status error = Status(
         (uint32_t)ErrorCode::kUndeclaredIdentifier, lldb::eErrorTypeGeneric,
         FormatDiagnostics(m_expr, errMsg, node->GetLocation()));
     return error.ToError();
   }
   lldb::ValueObjectSP val;
   lldb::TargetSP target_sp;
-  Status error;
 
   assert(identifier->GetKind() == IdentifierInfo::Kind::eValue &&
          "Unrecognized identifier kind");
@@ -263,10 +255,6 @@ Interpreter::Visit(const IdentifierNode *node) {
       return error.ToError();
   }
 
-  target_sp = val->GetTargetSP();
-  assert(target_sp && target_sp->IsValid() &&
-         "identifier doesn't resolve to a valid value");
-
   return val;
 }
 
diff --git a/lldb/source/ValueObject/DILParser.cpp b/lldb/source/ValueObject/DILParser.cpp
@@ -20,7 +20,7 @@
 #include <limits.h>
 #include <memory>
 #include <sstream>
-#include <stdlib.h>
+#include <cstdlib>
 #include <string>
 
 namespace lldb_private::dil {
@@ -29,7 +29,7 @@ std::string FormatDiagnostics(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
-  // overal text line).
+  // 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, message,
@@ -58,9 +58,7 @@ DILParser::DILParser(llvm::StringRef dil_input_expr, DILLexer lexer,
       m_check_ptr_vs_member(check_ptr_vs_member) {}
 
 llvm::Expected<ASTNodeUP> DILParser::Run() {
-  ASTNodeUP expr;
-
-  expr = ParseExpression();
+  ASTNodeUP expr = ParseExpression();
 
   Expect(Token::Kind::eof);
 
@@ -84,7 +82,7 @@ ASTNodeUP DILParser::ParseExpression() { return ParsePrimaryExpression(); }
 //    "(" expression ")"
 //
 ASTNodeUP DILParser::ParsePrimaryExpression() {
-  if (CurToken().IsOneOf(Token::coloncolon, Token::identifier)) {
+  if (CurToken().IsOneOf({Token::coloncolon, Token::identifier})) {
     // Save the source location for the diagnostics message.
     uint32_t loc = CurToken().GetLocation();
     auto identifier = ParseIdExpression();
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
@@ -20,42 +20,8 @@ class TestFrameVarDILGlobalVariableLookup(TestBase):
 
     def test_frame_var(self):
         self.build()
-        self.do_test()
-
-    def do_test(self):
-        target = self.createTestTarget()
-
-        # Now create a breakpoint in main.c at the source matching
-        # "Set a breakpoint here"
-        breakpoint = target.BreakpointCreateBySourceRegex(
-            "Set a breakpoint here", lldb.SBFileSpec("main.cpp")
-        )
-        self.assertTrue(
-            breakpoint and breakpoint.GetNumLocations() >= 1, VALID_BREAKPOINT
-        )
-
-        error = lldb.SBError()
-        # This is the launch info.  If you want to launch with arguments or
-        # environment variables, add them using SetArguments or
-        # SetEnvironmentEntries
-
-        launch_info = target.GetLaunchInfo()
-        process = target.Launch(launch_info, error)
-        self.assertTrue(process, PROCESS_IS_VALID)
-
-        # Did we hit our breakpoint?
-        from lldbsuite.test.lldbutil import get_threads_stopped_at_breakpoint
-
-        threads = get_threads_stopped_at_breakpoint(process, breakpoint)
-        self.assertEqual(
-            len(threads), 1, "There should be a thread stopped at our breakpoint"
-        )
-        # The hit count for the breakpoint should be 1.
-        self.assertEqual(breakpoint.GetHitCount(), 1)
-
-        frame = threads[0].GetFrameAtIndex(0)
-        command_result = lldb.SBCommandReturnObject()
-        interp = self.dbg.GetCommandInterpreter()
+        lldbutil.run_to_source_breakpoint(self, "Set a breakpoint here",
+                                          lldb.SBFileSpec("main.cpp"))
 
         self.expect("settings set target.experimental.use-DIL true", substrs=[""])
         self.expect("frame variable 'globalVar'", substrs=["-559038737"])  # 0xDEADBEEF
diff --git a/lldb/test/API/commands/frame/var-dil/basics/InstanceVariables/TestFrameVarDILInstanceVariables.py b/lldb/test/API/commands/frame/var-dil/basics/InstanceVariables/TestFrameVarDILInstanceVariables.py
@@ -20,42 +20,8 @@ class TestFrameVarDILInstanceVariables(TestBase):
 
     def test_frame_var(self):
         self.build()
-        self.do_test()
-
-    def do_test(self):
-        target = self.createTestTarget()
-
-        # Now create a breakpoint in main.c at the source matching
-        # "Set a breakpoint here"
-        breakpoint = target.BreakpointCreateBySourceRegex(
-            "Set a breakpoint here", lldb.SBFileSpec("main.cpp")
-        )
-        self.assertTrue(
-            breakpoint and breakpoint.GetNumLocations() >= 1, VALID_BREAKPOINT
-        )
-
-        error = lldb.SBError()
-        # This is the launch info.  If you want to launch with arguments or
-        # environment variables, add them using SetArguments or
-        # SetEnvironmentEntries
-
-        launch_info = target.GetLaunchInfo()
-        process = target.Launch(launch_info, error)
-        self.assertTrue(process, PROCESS_IS_VALID)
-
-        # Did we hit our breakpoint?
-        from lldbsuite.test.lldbutil import get_threads_stopped_at_breakpoint
-
-        threads = get_threads_stopped_at_breakpoint(process, breakpoint)
-        self.assertEqual(
-            len(threads), 1, "There should be a thread stopped at our breakpoint"
-        )
-        # The hit count for the breakpoint should be 1.
-        self.assertEqual(breakpoint.GetHitCount(), 1)
-
-        frame = threads[0].GetFrameAtIndex(0)
-        command_result = lldb.SBCommandReturnObject()
-        interp = self.dbg.GetCommandInterpreter()
+        lldbutil.run_to_source_breakpoint(self, "Set a breakpoint here",
+                                          lldb.SBFileSpec("main.cpp"))
 
         self.expect("settings set target.experimental.use-DIL true", substrs=[""])
         self.expect("frame variable 'this'", patterns=["0x[0-9]+"])
diff --git a/lldb/test/API/commands/frame/var-dil/basics/LocalVars/TestFrameVarDILLocalVars.py b/lldb/test/API/commands/frame/var-dil/basics/LocalVars/TestFrameVarDILLocalVars.py
@@ -20,42 +20,8 @@ class TestFrameVarDILLocalVars(TestBase):
 
     def test_frame_var(self):
         self.build()
-        self.do_test()
-
-    def do_test(self):
-        target = self.createTestTarget()
-
-        # Now create a breakpoint in main.c at the source matching
-        # "Set a breakpoint here"
-        breakpoint = target.BreakpointCreateBySourceRegex(
-            "Set a breakpoint here", lldb.SBFileSpec("main.cpp")
-        )
-        self.assertTrue(
-            breakpoint and breakpoint.GetNumLocations() >= 1, VALID_BREAKPOINT
-        )
-
-        error = lldb.SBError()
-        # This is the launch info.  If you want to launch with arguments or
-        # environment variables, add them using SetArguments or
-        # SetEnvironmentEntries
-
-        launch_info = target.GetLaunchInfo()
-        process = target.Launch(launch_info, error)
-        self.assertTrue(process, PROCESS_IS_VALID)
-
-        # Did we hit our breakpoint?
-        from lldbsuite.test.lldbutil import get_threads_stopped_at_breakpoint
-
-        threads = get_threads_stopped_at_breakpoint(process, breakpoint)
-        self.assertEqual(
-            len(threads), 1, "There should be a thread stopped at our breakpoint"
-        )
-        # The hit count for the breakpoint should be 1.
-        self.assertEqual(breakpoint.GetHitCount(), 1)
-
-        frame = threads[0].GetFrameAtIndex(0)
-        command_result = lldb.SBCommandReturnObject()
-        interp = self.dbg.GetCommandInterpreter()
+        lldbutil.run_to_source_breakpoint(self, "Set a breakpoint here",
+                                          lldb.SBFileSpec("main.cpp"))
 
         self.expect("settings set target.experimental.use-DIL true", substrs=[""])
         self.expect("frame variable a", substrs=["1"])
diff --git a/lldb/unittests/ValueObject/DILLexerTests.cpp b/lldb/unittests/ValueObject/DILLexerTests.cpp
@@ -54,9 +54,9 @@ TEST(DILLexerTests, TokenKindTest) {
 
   EXPECT_TRUE(token.Is(Token::identifier));
   EXPECT_FALSE(token.Is(Token::l_paren));
-  EXPECT_TRUE(token.IsOneOf(Token::eof, Token::identifier));
-  EXPECT_FALSE(token.IsOneOf(Token::l_paren, Token::r_paren, Token::coloncolon,
-                             Token::eof));
+  EXPECT_TRUE(token.IsOneOf({Token::eof, Token::identifier}));
+  EXPECT_FALSE(token.IsOneOf(
+      {Token::l_paren, Token::r_paren, Token::coloncolon, Token::eof}));
 }
 
 TEST(DILLexerTests, LookAheadTest) {
@@ -150,7 +150,7 @@ TEST(DILLexerTests, IdentifiersTest) {
     DILLexer lexer(*maybe_lexer);
     Token token = lexer.GetCurrentToken();
     EXPECT_TRUE(token.IsNot(Token::identifier));
-    EXPECT_TRUE(token.IsOneOf(Token::eof, Token::coloncolon, Token::l_paren,
-                              Token::r_paren));
+    EXPECT_TRUE(token.IsOneOf(
+        {Token::eof, Token::coloncolon, Token::l_paren, Token::r_paren}));
   }
 }

Original file line number	Diff line number	Diff line change
`@@ -13,7 +13,6 @@ namespace lldb_private::dil {`
`13`	`13`
`14`	`14`	`llvm::Expected<lldb::ValueObjectSP> ErrorNode::Accept(Visitor *v) const {`
`15`	`15`	`llvm_unreachable("Attempting to Visit a DIL ErrorNode.");`
`16`		`- return lldb::ValueObjectSP();`
`17`	`16`	`}`
`18`	`17`
`19`	`18`	`llvm::Expected<lldb::ValueObjectSP> IdentifierNode::Accept(Visitor *v) const {`