[binutils-gdb] Refactor expression completion

[binutils-gdb] Refactor expression completion

[Tom Tromey]

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=1e237aba2216f89b9a4b3235ad8d09d1b1b8f039

commit 1e237aba2216f89b9a4b3235ad8d09d1b1b8f039
Author: Tom Tromey <tromey@adacore.com>
Date:   Tue Feb 22 09:48:25 2022 -0700

    Refactor expression completion
    
    This refactors the gdb expression completion code to make it easier to
    add more types of completers.
    
    In the old approach, just two kinds of completers were supported:
    field names for some sub-expression, or tag names (like "enum
    something").  The data for each kind was combined in single structure,
    "expr_completion_state", and handled explicitly by
    complete_expression.
    
    In the new approach, the parser state just holds an object that is
    responsible for implementing completion.  This way, new completion
    types can be added by subclassing this base object.
    
    The structop completer is moved into structop_base_operation, and new
    objects are defined for use by the completion code.  This moves much
    of the logic of expression completion out of completer.c as well.

Diff:
---
 gdb/completer.c   | 92 +++++--------------------------------------------------
 gdb/eval.c        | 85 ++++++++++++++++++++++++++++++++++++++++++++++++++
 gdb/expop.h       | 12 +++-----
 gdb/expression.h  | 22 +++++++++++--
 gdb/parse.c       | 84 ++++++++++++++++++++++----------------------------
 gdb/parser-defs.h | 49 ++++++++++++++++++++++++-----
 6 files changed, 196 insertions(+), 148 deletions(-)

diff --git a/gdb/completer.c b/gdb/completer.c
index a51c16ac7f8..63b4cd22d24 100644
--- a/gdb/completer.c
+++ b/gdb/completer.c
@@ -1055,107 +1055,31 @@ location_completer_handle_brkchars (struct cmd_list_element *ignore,
   location_completer (ignore, tracker, text, NULL);
 }
 
-/* Helper for expression_completer which recursively adds field and
-   method names from TYPE, a struct or union type, to the OUTPUT
-   list.  */
-
-static void
-add_struct_fields (struct type *type, completion_list &output,
-		   const char *fieldname, int namelen)
-{
-  int i;
-  int computed_type_name = 0;
-  const char *type_name = NULL;
-
-  type = check_typedef (type);
-  for (i = 0; i < type->num_fields (); ++i)
-    {
-      if (i < TYPE_N_BASECLASSES (type))
-	add_struct_fields (TYPE_BASECLASS (type, i),
-			   output, fieldname, namelen);
-      else if (type->field (i).name ())
-	{
-	  if (type->field (i).name ()[0] != '\0')
-	    {
-	      if (! strncmp (type->field (i).name (), 
-			     fieldname, namelen))
-		output.emplace_back (xstrdup (type->field (i).name ()));
-	    }
-	  else if (type->field (i).type ()->code () == TYPE_CODE_UNION)
-	    {
-	      /* Recurse into anonymous unions.  */
-	      add_struct_fields (type->field (i).type (),
-				 output, fieldname, namelen);
-	    }
-	}
-    }
-
-  for (i = TYPE_NFN_FIELDS (type) - 1; i >= 0; --i)
-    {
-      const char *name = TYPE_FN_FIELDLIST_NAME (type, i);
-
-      if (name && ! strncmp (name, fieldname, namelen))
-	{
-	  if (!computed_type_name)
-	    {
-	      type_name = type->name ();
-	      computed_type_name = 1;
-	    }
-	  /* Omit constructors from the completion list.  */
-	  if (!type_name || strcmp (type_name, name))
-	    output.emplace_back (xstrdup (name));
-	}
-    }
-}
-
 /* See completer.h.  */
 
 void
 complete_expression (completion_tracker &tracker,
 		     const char *text, const char *word)
 {
-  struct type *type = NULL;
-  gdb::unique_xmalloc_ptr<char> fieldname;
-  enum type_code code = TYPE_CODE_UNDEF;
+  expression_up exp;
+  std::unique_ptr<expr_completion_base> expr_completer;
 
   /* Perform a tentative parse of the expression, to see whether a
      field completion is required.  */
   try
     {
-      type = parse_expression_for_completion (text, &fieldname, &code);
+      exp = parse_expression_for_completion (text, &expr_completer);
     }
   catch (const gdb_exception_error &except)
     {
       return;
     }
 
-  if (fieldname != nullptr && type)
-    {
-      for (;;)
-	{
-	  type = check_typedef (type);
-	  if (!type->is_pointer_or_reference ())
-	    break;
-	  type = TYPE_TARGET_TYPE (type);
-	}
-
-      if (type->code () == TYPE_CODE_UNION
-	  || type->code () == TYPE_CODE_STRUCT)
-	{
-	  completion_list result;
-
-	  add_struct_fields (type, result, fieldname.get (),
-			     strlen (fieldname.get ()));
-	  tracker.add_completions (std::move (result));
-	  return;
-	}
-    }
-  else if (fieldname != nullptr && code != TYPE_CODE_UNDEF)
-    {
-      collect_symbol_completion_matches_type (tracker, fieldname.get (),
-					      fieldname.get (), code);
-      return;
-    }
+  /* Part of the parse_expression_for_completion contract.  */
+  gdb_assert ((exp == nullptr) == (expr_completer == nullptr));
+  if (expr_completer != nullptr
+      && expr_completer->complete (exp.get (), tracker))
+    return;
 
   complete_files_symbols (tracker, text, word);
 }
diff --git a/gdb/eval.c b/gdb/eval.c
index 1211930a377..eded4845865 100644
--- a/gdb/eval.c
+++ b/gdb/eval.c
@@ -967,6 +967,91 @@ structop_base_operation::evaluate_funcall
 				  nullptr, expect_type);
 }
 
+/* Helper for structop_base_operation::complete which recursively adds
+   field and method names from TYPE, a struct or union type, to the
+   OUTPUT list.  */
+
+static void
+add_struct_fields (struct type *type, completion_list &output,
+		   const char *fieldname, int namelen)
+{
+  int i;
+  int computed_type_name = 0;
+  const char *type_name = NULL;
+
+  type = check_typedef (type);
+  for (i = 0; i < type->num_fields (); ++i)
+    {
+      if (i < TYPE_N_BASECLASSES (type))
+	add_struct_fields (TYPE_BASECLASS (type, i),
+			   output, fieldname, namelen);
+      else if (type->field (i).name ())
+	{
+	  if (type->field (i).name ()[0] != '\0')
+	    {
+	      if (! strncmp (type->field (i).name (),
+			     fieldname, namelen))
+		output.emplace_back (xstrdup (type->field (i).name ()));
+	    }
+	  else if (type->field (i).type ()->code () == TYPE_CODE_UNION)
+	    {
+	      /* Recurse into anonymous unions.  */
+	      add_struct_fields (type->field (i).type (),
+				 output, fieldname, namelen);
+	    }
+	}
+    }
+
+  for (i = TYPE_NFN_FIELDS (type) - 1; i >= 0; --i)
+    {
+      const char *name = TYPE_FN_FIELDLIST_NAME (type, i);
+
+      if (name && ! strncmp (name, fieldname, namelen))
+	{
+	  if (!computed_type_name)
+	    {
+	      type_name = type->name ();
+	      computed_type_name = 1;
+	    }
+	  /* Omit constructors from the completion list.  */
+	  if (!type_name || strcmp (type_name, name))
+	    output.emplace_back (xstrdup (name));
+	}
+    }
+}
+
+/* See expop.h.  */
+
+bool
+structop_base_operation::complete (struct expression *exp,
+				   completion_tracker &tracker)
+{
+  const std::string &fieldname = std::get<1> (m_storage);
+
+  value *lhs = std::get<0> (m_storage)->evaluate (nullptr, exp,
+						  EVAL_AVOID_SIDE_EFFECTS);
+  struct type *type = value_type (lhs);
+  for (;;)
+    {
+      type = check_typedef (type);
+      if (!type->is_pointer_or_reference ())
+	break;
+      type = TYPE_TARGET_TYPE (type);
+    }
+
+  if (type->code () == TYPE_CODE_UNION
+      || type->code () == TYPE_CODE_STRUCT)
+    {
+      completion_list result;
+
+      add_struct_fields (type, result, fieldname.c_str (),
+			 fieldname.length ());
+      tracker.add_completions (std::move (result));
+      return true;
+    }
+
+  return false;
+}
 
 } /* namespace expr */
 
diff --git a/gdb/expop.h b/gdb/expop.h
index c8c1aa1b8e8..c159d96a561 100644
--- a/gdb/expop.h
+++ b/gdb/expop.h
@@ -997,18 +997,16 @@ public:
     return std::get<1> (m_storage);
   }
 
-  /* Used for completion.  Evaluate the LHS for type.  */
-  value *evaluate_lhs (struct expression *exp)
-  {
-    return std::get<0> (m_storage)->evaluate (nullptr, exp,
-					      EVAL_AVOID_SIDE_EFFECTS);
-  }
-
   value *evaluate_funcall (struct type *expect_type,
 			   struct expression *exp,
 			   enum noside noside,
 			   const std::vector<operation_up> &args) override;
 
+  /* Try to complete this operation in the context of EXP.  TRACKER is
+     the completion tracker to update.  Return true if completion was
+     possible, false otherwise.  */
+  bool complete (struct expression *exp, completion_tracker &tracker);
+
 protected:
 
   using tuple_holding_operation::tuple_holding_operation;
diff --git a/gdb/expression.h b/gdb/expression.h
index ff3990e979c..3ba68a2db93 100644
--- a/gdb/expression.h
+++ b/gdb/expression.h
@@ -232,8 +232,26 @@ extern expression_up parse_expression (const char *,
 extern expression_up parse_expression_with_language (const char *string,
 						     enum language lang);
 
-extern struct type *parse_expression_for_completion
-    (const char *, gdb::unique_xmalloc_ptr<char> *, enum type_code *);
+
+class completion_tracker;
+
+/* Base class for expression completion.  An instance of this
+   represents a completion request from the parser.  */
+struct expr_completion_base
+{
+  /* Perform this object's completion.  EXP is the expression in which
+     the completion occurs.  TRACKER is the tracker to update with the
+     results.  Return true if completion was possible (even if no
+     completions were found), false to fall back to ordinary
+     expression completion (i.e., symbol names).  */
+  virtual bool complete (struct expression *exp,
+			 completion_tracker &tracker) = 0;
+
+  virtual ~expr_completion_base () = default;
+};
+
+extern expression_up parse_expression_for_completion
+     (const char *, std::unique_ptr<expr_completion_base> *completer);
 
 class innermost_block_tracker;
 extern expression_up parse_exp_1 (const char **, CORE_ADDR pc,
diff --git a/gdb/parse.c b/gdb/parse.c
index 8886ab0eb04..7637e5799f7 100644
--- a/gdb/parse.c
+++ b/gdb/parse.c
@@ -72,10 +72,11 @@ show_parserdebug (struct ui_file *file, int from_tty,
 }
 
 
-static expression_up parse_exp_in_context (const char **, CORE_ADDR,
-					   const struct block *, int,
-					   bool, innermost_block_tracker *,
-					   expr_completion_state *);
+static expression_up parse_exp_in_context
+     (const char **, CORE_ADDR,
+      const struct block *, int,
+      bool, innermost_block_tracker *,
+      std::unique_ptr<expr_completion_base> *);
 
 /* Documented at it's declaration.  */
 
@@ -171,15 +172,22 @@ find_minsym_type_and_address (minimal_symbol *msymbol,
     }
 }
 
+bool
+expr_complete_tag::complete (struct expression *exp,
+			     completion_tracker &tracker)
+{
+  collect_symbol_completion_matches_type (tracker, m_name.get (),
+					  m_name.get (), m_code);
+  return true;
+}
+
 /* See parser-defs.h.  */
 
 void
 parser_state::mark_struct_expression (expr::structop_base_operation *op)
 {
-  gdb_assert (parse_completion
-	      && (m_completion_state.expout_tag_completion_type
-		  == TYPE_CODE_UNDEF));
-  m_completion_state.expout_last_op = op;
+  gdb_assert (parse_completion && m_completion_state == nullptr);
+  m_completion_state.reset (new expr_complete_structop (op));
 }
 
 /* Indicate that the current parser invocation is completing a tag.
@@ -190,17 +198,12 @@ void
 parser_state::mark_completion_tag (enum type_code tag, const char *ptr,
 				   int length)
 {
-  gdb_assert (parse_completion
-	      && (m_completion_state.expout_tag_completion_type
-		  == TYPE_CODE_UNDEF)
-	      && m_completion_state.expout_completion_name == NULL
-	      && m_completion_state.expout_last_op == nullptr);
+  gdb_assert (parse_completion && m_completion_state == nullptr);
   gdb_assert (tag == TYPE_CODE_UNION
 	      || tag == TYPE_CODE_STRUCT
 	      || tag == TYPE_CODE_ENUM);
-  m_completion_state.expout_tag_completion_type = tag;
-  m_completion_state.expout_completion_name
-    = make_unique_xstrndup (ptr, length);
+  m_completion_state.reset
+    (new expr_complete_tag (tag, make_unique_xstrndup (ptr, length)));
 }
 
 /* See parser-defs.h.  */
@@ -433,7 +436,7 @@ parse_exp_in_context (const char **stringptr, CORE_ADDR pc,
 		      const struct block *block,
 		      int comma, bool void_context_p,
 		      innermost_block_tracker *tracker,
-		      expr_completion_state *cstate)
+		      std::unique_ptr<expr_completion_base> *completer)
 {
   const struct language_defn *lang = NULL;
 
@@ -501,7 +504,7 @@ parse_exp_in_context (const char **stringptr, CORE_ADDR pc,
 
   parser_state ps (lang, get_current_arch (), expression_context_block,
 		   expression_context_pc, comma, *stringptr,
-		   cstate != nullptr, tracker, void_context_p);
+		   completer != nullptr, tracker, void_context_p);
 
   scoped_restore_current_language lang_saver;
   set_language (lang->la_language);
@@ -525,8 +528,8 @@ parse_exp_in_context (const char **stringptr, CORE_ADDR pc,
   if (expressiondebug)
     dump_prefix_expression (result.get (), gdb_stdlog);
 
-  if (cstate != nullptr)
-    *cstate = std::move (ps.m_completion_state);
+  if (completer != nullptr)
+    *completer = std::move (ps.m_completion_state);
   *stringptr = ps.lexptr;
   return result;
 }
@@ -566,47 +569,32 @@ parse_expression_with_language (const char *string, enum language lang)
   return parse_expression (string);
 }
 
-/* Parse STRING as an expression.  If parsing ends in the middle of a
-   field reference, return the type of the left-hand-side of the
-   reference; furthermore, if the parsing ends in the field name,
-   return the field name in *NAME.  If the parsing ends in the middle
-   of a field reference, but the reference is somehow invalid, throw
-   an exception.  In all other cases, return NULL.  */
-
-struct type *
-parse_expression_for_completion (const char *string,
-				 gdb::unique_xmalloc_ptr<char> *name,
-				 enum type_code *code)
+/* Parse STRING as an expression.  If the parse is marked for
+   completion, set COMPLETER and return the expression.  In all other
+   cases, return NULL.  */
+
+expression_up
+parse_expression_for_completion
+     (const char *string,
+      std::unique_ptr<expr_completion_base> *completer)
 {
   expression_up exp;
-  expr_completion_state cstate;
 
   try
     {
-      exp = parse_exp_in_context (&string, 0, 0, 0, false, nullptr, &cstate);
+      exp = parse_exp_in_context (&string, 0, 0, 0, false, nullptr, completer);
     }
   catch (const gdb_exception_error &except)
     {
       /* Nothing, EXP remains NULL.  */
     }
 
-  if (exp == NULL)
-    return NULL;
-
-  if (cstate.expout_tag_completion_type != TYPE_CODE_UNDEF)
-    {
-      *code = cstate.expout_tag_completion_type;
-      *name = std::move (cstate.expout_completion_name);
-      return NULL;
-    }
-
-  if (cstate.expout_last_op == nullptr)
+  /* If we didn't get a completion result, be sure to also not return
+     an expression to our caller.  */
+  if (*completer == nullptr)
     return nullptr;
 
-  expr::structop_base_operation *op = cstate.expout_last_op;
-  const std::string &fld = op->get_string ();
-  *name = make_unique_xstrdup (fld.c_str ());
-  return value_type (op->evaluate_lhs (exp.get ()));
+  return exp;
 }
 
 /* Parse floating point value P of length LEN.
diff --git a/gdb/parser-defs.h b/gdb/parser-defs.h
index 6de514023b2..71381b1a725 100644
--- a/gdb/parser-defs.h
+++ b/gdb/parser-defs.h
@@ -82,20 +82,55 @@ struct expr_builder
   expression_up expout;
 };
 
-/* This is used for expression completion.  */
+/* Complete an expression that references a field, like "x->y".  */
 
-struct expr_completion_state
+struct expr_complete_structop : public expr_completion_base
 {
+  explicit expr_complete_structop (expr::structop_base_operation *op)
+    : m_op (op)
+  {
+  }
+
+  bool complete (struct expression *exp,
+		 completion_tracker &tracker) override
+  {
+    return m_op->complete (exp, tracker);
+  }
+
+private:
+
   /* The last struct expression directly before a '.' or '->'.  This
      is set when parsing and is only used when completing a field
      name.  It is nullptr if no dereference operation was found.  */
-  expr::structop_base_operation *expout_last_op = nullptr;
+  expr::structop_base_operation *m_op = nullptr;
+};
+
+/* Complete a tag name in an expression.  This is used for something
+   like "enum abc<TAB>".  */
+
+struct expr_complete_tag : public expr_completion_base
+{
+  expr_complete_tag (enum type_code code,
+		     gdb::unique_xmalloc_ptr<char> name)
+    : m_code (code),
+      m_name (std::move (name))
+  {
+    /* Parsers should enforce this statically.  */
+    gdb_assert (code == TYPE_CODE_ENUM
+		|| code == TYPE_CODE_UNION
+		|| code == TYPE_CODE_STRUCT);
+  }
+
+  bool complete (struct expression *exp,
+		 completion_tracker &tracker) override;
+
+private:
 
-  /* If we are completing a tagged type name, this will be nonzero.  */
-  enum type_code expout_tag_completion_type = TYPE_CODE_UNDEF;
+  /* The kind of tag to complete.  */
+  enum type_code m_code;
 
   /* The token for tagged type name completion.  */
-  gdb::unique_xmalloc_ptr<char> expout_completion_name;
+  gdb::unique_xmalloc_ptr<char> m_name;
 };
 
 /* An instance of this type is instantiated during expression parsing,
@@ -254,7 +289,7 @@ struct parser_state : public expr_builder
   bool parse_completion;
 
   /* Completion state is updated here.  */
-  expr_completion_state m_completion_state;
+  std::unique_ptr<expr_completion_base> m_completion_state;
 
   /* The innermost block tracker.  */
   innermost_block_tracker *block_tracker;