This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] Use visitors for make_gdb_type
- From: Simon Marchi <simark at simark dot ca>
- To: Alan Hayward <Alan dot Hayward at arm dot com>, "gdb-patches at sourceware dot org" <gdb-patches at sourceware dot org>
- Cc: nd <nd at arm dot com>
- Date: Sun, 28 Jan 2018 21:24:19 -0500
- Subject: Re: [PATCH] Use visitors for make_gdb_type
- Authentication-results: sourceware.org; auth=none
- References: <a06ab31b-0144-82fe-5d29-4e81016990d3@arm.com> <F07C9294-69B0-4735-A13E-5D8FF608C8C6@arm.com>
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