From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 12826 invoked by alias); 29 Jan 2018 02:24:32 -0000 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 Received: (qmail 12778 invoked by uid 89); 29 Jan 2018 02:24:24 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,SPF_HELO_PASS,SPF_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy= X-HELO: simark.ca Received: from simark.ca (HELO simark.ca) (158.69.221.121) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 29 Jan 2018 02:24:21 +0000 Received: from [10.0.0.11] (192-222-251-162.qc.cable.ebox.net [192.222.251.162]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id CFF7B1E48F; Sun, 28 Jan 2018 21:24:19 -0500 (EST) Subject: Re: [PATCH] Use visitors for make_gdb_type To: Alan Hayward , "gdb-patches@sourceware.org" Cc: nd References: From: Simon Marchi Message-ID: Date: Mon, 29 Jan 2018 02:24:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.2 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2018-01/txt/msg00580.txt.bz2 On 2018-01-26 10:30 AM, Alan Hayward wrote: > I appear to still have email issues - previous post had the tabs stripped out. > Hoping this version is ok. Apologies. Hi Alan, I was able to apply it correctly. > This patch implements the suggestion in the review for > [PATCH v2 6/8] Create xml from target descriptions. > > Remove the make_gdb_type functions from the tdesc_type_ classes. > Replace with a static make_gdb_type function that uses a element > visitor called gdb_type_creator. > > I've defined gdb_type_creator inside make_gdb_type because it shouldn't > be needed outside the function. > > The method of creating the types has not changed. > > This patch will allow a future patch to commonise the tdesc_types with > gdbserver without having to move any gdb_type functionality. > > Alan. make_gdb_type_union & co could be methods of gdb_type_creator and access gdbarch directly. I think it would make sense to have all the knowledge of the tdesc type to gdb type conversion inside that class. > - type *make_gdb_type (struct gdbarch *gdbarch) const override > +static type * > +make_gdb_type (struct gdbarch *gdbarch, struct tdesc_type *ttype) > +{ > + class gdb_type_creator : public tdesc_element_visitor > { > - type *gdb_type = tdesc_find_type (gdbarch, this->name.c_str ()); > - if (gdb_type != NULL) > - return gdb_type; > + public: > + gdb_type_creator (struct gdbarch *gdbarch) > + : m_gdbarch (gdbarch) > + {} > > - switch (this->kind) > + type *get_type () > { > - case TDESC_TYPE_STRUCT: > - return make_gdb_type_struct (gdbarch); > - case TDESC_TYPE_UNION: > - return make_gdb_type_union (gdbarch); > - case TDESC_TYPE_FLAGS: > - return make_gdb_type_flags (gdbarch); > - case TDESC_TYPE_ENUM: > - return make_gdb_type_enum (gdbarch); > + return m_type; > } > > - internal_error (__FILE__, __LINE__, > - "Type \"%s\" has an unknown kind %d", > - this->name.c_str (), this->kind); > + void visit_pre (const target_desc *e) > + {} I think we should have empty default implementations of these visit functions in the base class, instead of having to declare them empty when unnecessary. Maybe Yao had a reason not to do this initially? In any case, make sure to mark the overriden methods with the "override" keyword. Using clang: /home/simark/src/binutils-gdb/gdb/target-descriptions.c:416:10: error: 'visit_pre' overrides a member function but is not marked 'override' [-Werror,-Winconsistent-missing-override] void visit_pre (const target_desc *e) ^ /home/simark/src/binutils-gdb/gdb/target-descriptions.c:48:16: note: overridden virtual function is here virtual void visit_pre (const target_desc *e) = 0; ^ /home/simark/src/binutils-gdb/gdb/target-descriptions.c:419:10: error: 'visit_post' overrides a member function but is not marked 'override' [-Werror,-Winconsistent-missing-override] void visit_post (const target_desc *e) ^ /home/simark/src/binutils-gdb/gdb/target-descriptions.c:49:16: note: overridden virtual function is here virtual void visit_post (const target_desc *e) = 0; ^ /home/simark/src/binutils-gdb/gdb/target-descriptions.c:422:10: error: 'visit_pre' overrides a member function but is not marked 'override' [-Werror,-Winconsistent-missing-override] void visit_pre (const tdesc_feature *e) ^ /home/simark/src/binutils-gdb/gdb/target-descriptions.c:51:16: note: overridden virtual function is here virtual void visit_pre (const tdesc_feature *e) = 0; ^ /home/simark/src/binutils-gdb/gdb/target-descriptions.c:425:10: error: 'visit_post' overrides a member function but is not marked 'override' [-Werror,-Winconsistent-missing-override] void visit_post (const tdesc_feature *e) ^ /home/simark/src/binutils-gdb/gdb/target-descriptions.c:52:16: note: overridden virtual function is here virtual void visit_post (const tdesc_feature *e) = 0; ^ /home/simark/src/binutils-gdb/gdb/target-descriptions.c:544:10: error: 'visit' overrides a member function but is not marked 'override' [-Werror,-Winconsistent-missing-override] void visit (const tdesc_reg *reg) ^ /home/simark/src/binutils-gdb/gdb/target-descriptions.c:58:16: note: overridden virtual function is here virtual void visit (const tdesc_reg *e) = 0; ^ Otherwise, LGTM. Thanks, Simon