From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 26199 invoked by alias); 21 Oct 2008 22:25:42 -0000 Received: (qmail 26187 invoked by uid 22791); 21 Oct 2008 22:25:41 -0000 X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (65.74.133.4) by sourceware.org (qpsmtpd/0.31) with ESMTP; Tue, 21 Oct 2008 22:24:38 +0000 Received: (qmail 23364 invoked from network); 21 Oct 2008 22:24:36 -0000 Received: from unknown (HELO orlando.local) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 21 Oct 2008 22:24:36 -0000 From: Pedro Alves To: gdb-patches@sourceware.org, tromey@redhat.com Subject: Re: RFA: fix PR gdb/2489 Date: Tue, 21 Oct 2008 22:25:00 -0000 User-Agent: KMail/1.9.9 References: In-Reply-To: MIME-Version: 1.0 Content-Disposition: inline Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Message-Id: <200810212324.38183.pedro@codesourcery.com> X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2008-10/txt/msg00526.txt.bz2 On Friday 01 August 2008 20:14:50, Tom Tromey wrote: > This patch fixes PR gdb/2489. >=20 > The bug is that field name completion does not consider methods. >=20 > The fix is to also examine method names when completing; but not to > consider constructor names. >=20 > b/gdb/ChangeLog: > 2008-08-01 Tom Tromey >=20 > PR gdb/2489: > * completer.c (count_struct_fields): Count method names. > (add_struct_fields): Add matching method names. >=20 > b/gdb/testsuite/ChangeLog: > 2008-08-01 Tom Tromey >=20 > * gdb.cp/pr2489.cc: New file. > * gdb.cp/cpcompletion.exp: New file. >=20 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 =3D TYPE_NFN_FIELDS (type) - 1; i >=3D0; --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 =3D TYPE_NFN_FIELDS (type) - 1; i >=3D0; --i) + { + char *name =3D TYPE_FN_FIELDLIST_NAME (type, i); + if (name && ! strncmp (name, fieldname, namelen)) + { + if (!type_name) + type_name =3D 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" ob= ject {c++ debug}] !=3D ""} { > + untested completion.exp > + return -1 > +} > + > +if {[gdb_compile "${testfile}.o" ${binfile} executable {c++ debug}] !=3D= "" } { > + 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 "^.* =3D 0.*$gdb_prompt $"\ > + { pass "complete 'p foo1.g'"} > + -re ".*$gdb_prompt $" { fail "complete 'p foo1.g'"} > + timeout {fail "(timeout) complete 'p foo= 1.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; =46rom my testing, foo1.g will make get_foo only show up once, which is fine, IMO. A possible *future* improvement would be go show: foo1. 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 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. foo1.FooBase::get_foo foo1.get_foo Or: foo1.g foo1.get_foo foo1.get_foo( would make things clearer. -- Anonymous struct with method, struct { int get_foo () { ... } } a; a. 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. --=20 Pedro Alves