Re: RFA: fix PR gdb/2489

[Tom Tromey <tromey at redhat dot com>]

>>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:

Finally getting back to this old patch...

Tom> This patch fixes PR gdb/2489.
Tom> The bug is that field name completion does not consider methods.

Tom> +  for (i = TYPE_NFN_FIELDS (type) - 1; i >=0; --i)
Pedro>                                               ^ missing space

Fixed.

Pedro> +  for (i = TYPE_NFN_FIELDS (type) - 1; i >=0; --i)
Pedro> +    {
Pedro> +      char *name = TYPE_FN_FIELDLIST_NAME (type, i);
Pedro> +      if (name && ! strncmp (name, fieldname, namelen))
Pedro> +       {
Pedro> +         if (!type_name)
Pedro> +           type_name = type_name_no_tag (type);
Pedro> +         /* Omit constructors from the completion list.  */
Pedro> +         if (strcmp (type_name, name))
Pedro> +           {

Pedro> Can type_name ever be NULL here then?

The reason for the check here is that we compute type_name the first
time we need it.  It is initialized to NULL, but only set once.

Tom> +# Please email any bugs, comments, and/or additions to this file to:
Tom> +# bug-gdb@prep.ai.mit.edu

Pedro> Please remove this bit.

Whoops, fixed.

Tom> +     untested completion.exp

Pedro> s/completions.exp/cpcompletion.exp/

Thanks -- fixed.

Pedro> (I wish we had a function we could call that abstracted and
Pedro> made easier to write/read these completion tests.)

I rewrote the tests to use the "complete" command rather than sending
a TAB.  This makes them much simpler.

Pedro> What do you think about extending the test a little bit?

Pedro> I think it would be nice to have,
Pedro> Plain inheritance testing that the base class methods are completed.
[...]
Pedro> Inheritance + masking, something like:
[...]
Pedro> Anonymous struct with method,
[...]
Pedro> Also, would it make sense to add a test that made sure the
Pedro> ctors aren't completed?

I added these, thanks for the suggestions.

I've appended the final patch.  I am checking this in.

Tom

2009-02-02  Tom Tromey  <tromey@redhat.com>

	PR gdb/2489:
	* completer.c (count_struct_fields): Count method names.
	(add_struct_fields): Add matching method names.

2009-02-02  Tom Tromey  <tromey@redhat.com>

	* gdb.cp/Makefile.in (EXECUTABLES): Add pr2489.
	* gdb.cp/pr2489.cc: New file.
	* gdb.cp/cpcompletion.exp: New file.

Index: completer.c
===================================================================
RCS file: /cvs/src/src/gdb/completer.c,v
retrieving revision 1.30
diff -u -r1.30 completer.c
--- completer.c	3 Jan 2009 05:57:51 -0000	1.30
+++ completer.c	3 Feb 2009 00:56:41 -0000
@@ -339,7 +339,7 @@
 }
 
 /* Helper for expression_completer which recursively counts the number
-   of named fields in a structure or union type.  */
+   of named fields and methods in a structure or union type.  */
 static int
 count_struct_fields (struct type *type)
 {
@@ -353,17 +353,25 @@
       else if (TYPE_FIELD_NAME (type, i))
 	++result;
     }
+
+  for (i = TYPE_NFN_FIELDS (type) - 1; i >= 0; --i)
+    {
+      if (TYPE_FN_FIELDLIST_NAME (type, i))
+	++result;
+    }
+
   return result;
 }
 
-/* Helper for expression_completer which recursively adds field names
-   from TYPE, a struct or union type, to the array OUTPUT.  This
-   function assumes that OUTPUT is correctly-sized.  */
+/* Helper for expression_completer which recursively adds field and
+   method names from TYPE, a struct or union type, to the array
+   OUTPUT.  This function assumes that OUTPUT is correctly-sized.  */
 static void
 add_struct_fields (struct type *type, int *nextp, char **output,
 		   char *fieldname, int namelen)
 {
   int i;
+  char *type_name = NULL;
 
   CHECK_TYPEDEF (type);
   for (i = 0; i < TYPE_NFIELDS (type); ++i)
@@ -378,6 +386,22 @@
 	  ++*nextp;
 	}
     }
+
+  for (i = TYPE_NFN_FIELDS (type) - 1; i >= 0; --i)
+    {
+      char *name = TYPE_FN_FIELDLIST_NAME (type, i);
+      if (name && ! strncmp (name, fieldname, namelen))
+	{
+	  if (!type_name)
+	    type_name = type_name_no_tag (type);
+	  /* Omit constructors from the completion list.  */
+	  if (strcmp (type_name, name))
+	    {
+	      output[*nextp] = xstrdup (name);
+	      ++*nextp;
+	    }
+	}
+    }
 }
 
 /* Complete on expressions.  Often this means completing on symbol
Index: testsuite/gdb.cp/Makefile.in
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.cp/Makefile.in,v
retrieving revision 1.3
diff -u -r1.3 Makefile.in
--- testsuite/gdb.cp/Makefile.in	28 Mar 2007 00:32:41 -0000	1.3
+++ testsuite/gdb.cp/Makefile.in	3 Feb 2009 00:56:43 -0000
@@ -4,7 +4,7 @@
 EXECUTABLES = ambiguous annota2 anon-union cplusfuncs cttiadd \
 	derivation inherit local member-ptr method misc \
         overload ovldbreak ref-typ ref-typ2 templates userdef virtfunc namespace \
-	ref-types ref-params method2
+	ref-types ref-params method2 pr2489
 
 all info install-info dvi install uninstall installcheck check:
 	@echo "Nothing to be done for $@..."
Index: testsuite/gdb.cp/cpcompletion.exp
===================================================================
RCS file: testsuite/gdb.cp/cpcompletion.exp
diff -N testsuite/gdb.cp/cpcompletion.exp
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/gdb.cp/cpcompletion.exp	3 Feb 2009 00:56:43 -0000
@@ -0,0 +1,72 @@
+# Copyright 2009 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# This file is part of the gdb testsuite.
+
+if $tracelevel then {
+	strace $tracelevel
+}
+
+if { [skip_cplus_tests] } { continue }
+
+set testfile pr2489
+set binfile ${objdir}/${subdir}/${testfile}
+
+if {[gdb_compile "${srcdir}/${subdir}/${testfile}.cc" "${testfile}.o" object {c++ debug}] != ""} {
+    untested cpcompletion.exp
+    return -1
+}
+
+if {[gdb_compile "${testfile}.o" ${binfile} executable {c++ debug}] != "" } {
+     untested cpcompletion.exp
+     return -1
+}
+
+gdb_exit
+
+# Don't let a .inputrc file or an existing setting of INPUTRC mess up
+# the test results.  Even if /dev/null doesn't exist on the particular
+# platform, the readline library will use the default setting just by
+# failing to open the file.  OTOH, opening /dev/null successfully will
+# also result in the default settings being used since nothing will be
+# read from this file.
+global env
+if [info exists env(INPUTRC)] {
+    set old_inputrc $env(INPUTRC)
+}
+set env(INPUTRC) "/dev/null"
+
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${binfile}
+
+set bp_location [gdb_get_line_number "Set breakpoint here" ${testfile}.cc]
+
+if {![runto $bp_location]} {
+    perror "test suppressed"
+}
+
+# This also tests inheritance -- completion should only see a single
+# "get_foo".
+gdb_test "complete p foo1.g" "p foo1\\.get_foo"
+
+# Test inheritance without overriding.
+gdb_test "complete p foo1.base" "p foo1\\.base_function_only"
+
+# Test non-completion of constructor names.
+gdb_test "complete p foo1.Fo" "p foo1\\.Foofoo"
+
+# Test completion with an anonymous struct.
+gdb_test "complete p a.g" "p a\\.get"
Index: testsuite/gdb.cp/pr2489.cc
===================================================================
RCS file: testsuite/gdb.cp/pr2489.cc
diff -N testsuite/gdb.cp/pr2489.cc
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/gdb.cp/pr2489.cc	3 Feb 2009 00:56:43 -0000
@@ -0,0 +1,51 @@
+
+class Base
+{
+public:
+  virtual int get_foo () { return 1; }
+  int base_function_only () { return 2; }
+};
+
+class Foo : public Base
+{
+
+private:
+  int foo_value;
+
+public:
+  Foo () { foo_value = 0;}
+  Foo (int i) { foo_value = i;}
+  ~Foo () { }
+  void set_foo (int value);
+  int get_foo ();
+
+  // Something similar to a constructor name.
+  void Foofoo ();
+
+  bool operator== (const Foo &other) { return foo_value == other.foo_value; }
+};
+ 
+void Foo::set_foo (int value)
+{
+  foo_value = value;
+}
+
+int Foo::get_foo ()
+{
+  return foo_value;
+}
+
+void Foo::Foofoo ()
+{
+}
+
+int main ()
+{
+  // Anonymous struct with method.
+  struct {
+    int get() { return 5; }
+  } a;
+  Foo foo1;
+  foo1.set_foo (42);		// Set breakpoint here.
+  return 0;
+}