This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: RFA: fix PR gdb/2489
- From: Pedro Alves <pedro at codesourcery dot com>
- To: gdb-patches at sourceware dot org, tromey at redhat dot com
- Date: Tue, 21 Oct 2008 23:24:37 +0100
- Subject: Re: RFA: fix PR gdb/2489
- References: <m3y73gy2c5.fsf@fleche.redhat.com>
On Friday 01 August 2008 20:14:50, Tom Tromey wrote:
> This patch fixes PR gdb/2489.
>
> The bug is that field name completion does not consider methods.
>
> The fix is to also examine method names when completing; but not to
> consider constructor names.
>
> b/gdb/ChangeLog:
> 2008-08-01 Tom Tromey <tromey@redhat.com>
>
> PR gdb/2489:
> * completer.c (count_struct_fields): Count method names.
> (add_struct_fields): Add matching method names.
>
> b/gdb/testsuite/ChangeLog:
> 2008-08-01 Tom Tromey <tromey@redhat.com>
>
> * gdb.cp/pr2489.cc: New file.
> * gdb.cp/cpcompletion.exp: New file.
>
This looks mostly OK to me. This patch is pending for a few months
now, so it shouldn't hurt to wait a couple of days more to give the
C++ maintainer a chance to comment. :-)
> + for (i = TYPE_NFN_FIELDS (type) - 1; i >=0; --i)
^ missing space
We have this description in gdbtypes.c:
/* Return a typename for a struct/union/enum type without "struct ",
"union ", or "enum ". If the type has a NULL name, return NULL. */
char *
type_name_no_tag (const struct type *type)
{
+ 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))
+ {
Can type_name ever be NULL here then? I'm no guru in this
area, but I think not.
> +# Please email any bugs, comments, and/or additions to this file to:
> +# bug-gdb@prep.ai.mit.edu
Please remove this bit. Jan Kratochvil has recently gone through removing
all references to this long gone address. (It seems a couple have crept
in since though.)
> +
> +if {[gdb_compile "${srcdir}/${subdir}/${testfile}.cc" "${testfile}.o" object {c++ debug}] != ""} {
> + untested completion.exp
> + return -1
> +}
> +
> +if {[gdb_compile "${testfile}.o" ${binfile} executable {c++ debug}] != "" } {
> + untested completion.exp
> + return -1
> +}
s/completions.exp/cpcompletion.exp/
> +
> +send_gdb "p foo1.g\t"
> +gdb_expect {
> + -re "^p foo1\\.get_foo $"\
> + { send_gdb "()\n"
> + gdb_expect {
> + -re "^.* = 0.*$gdb_prompt $"\
> + { pass "complete 'p foo1.g'"}
> + -re ".*$gdb_prompt $" { fail "complete 'p foo1.g'"}
> + timeout {fail "(timeout) complete 'p foo1.g'"}
> + }
> + }
> + -re ".*$gdb_prompt $" { fail "complete 'p foo1.g'" }
> + timeout { fail "(timeout) complete 'p foo1.g' 2" }
> + }
> +
(I wish we had a function we could call that abstracted and made easier to
write/read these completion tests.)
What do you think about extending the test a little bit?
I think it would be nice to have,
Plain inheritance testing that the base class methods are completed.
--
Inheritance + masking, something like:
class FooBase
{
public:
int get_foo () { return 1; }
}
class Foo : public FooBase
{
public:
int get_foo () { ... }
}
Foo foo1;
From my testing, foo1.g<tab> will make get_foo only show up once,
which is fine, IMO.
A possible *future* improvement would be go show:
foo1.<tab>
foo1.FooBase::get_foo
foo1.get_foo
(foo1.Foo::get_foo too perhaps, not sure)
--
Masking, changing type:
class FooBase
{
private:
int get_foo;
}
class Foo : public FooBase
{
public:
int get_foo () { ... }
}
Foo foo1;
This case, foo1.g<tab> completes to foo1.get_foo, but 'p foo1.get_foo'
prints FooBase::get_foo. It's a bit confusing, especially since
get_foo is private in the base class --- not related to your patch,
just pointing it out. This is somewhat related to the future improvement I
mentioned above. That is, maybe showing:
foo1.<tab>
foo1.FooBase::get_foo
foo1.get_foo
Or:
foo1.g<tab>
foo1.get_foo
foo1.get_foo(
would make things clearer.
--
Anonymous struct with method,
struct
{
int get_foo () { ... }
} a;
a.<tab>
get_foo
--
Also, would it make sense to add a test that made sure the
ctors aren't completed?
--
Consider any test additions you make pre-approved.
--
Pedro Alves