Re: [PATCH 11/10] Split tdesc_type into multiple classes

Simon Marchi
2017-12-05T21:42:00Z

On 2017-12-05 11:46 AM, Yao Qi wrote:
> Simon Marchi <simon.marchi@polymtl.ca> writes:
>
>> Hi Yao,
>>
>> I revisited this patch, and implemented as best as I could what you
>> suggested. This patch applies on top of the current series.
>>
>> This patch makes tdesc_type an abstract base class and creates three
>> subclasses:
>>
>> - tdesc_type_builtin, for builtin types
>> - tdesc_type_vector, for vector types
>> - tdesc_type_with_fields, for struct, union, flag and enum types
>>
>> This allows getting rid of the union in tdesc_type and to not allow the
>> std::vector separately. I tried to go further and create separate
>> classes for struct, union, flag and enum, but it proved too difficult.
>> One problem is that from the point of the of the target description
>> code, the types tdesc_type_* are opaque (only forward-declared).
>> Therefore, it doesn't know about inheritance relationship between those
>> classes. This makes it impossible to make functions that accept a
>> pointer to a base class and pass a pointer to a derived class, for
>> example. I think this patch here is a good compromise, and if somebody
>> wants to improve things further, the door is open.
>>
>> A make_gdb_type virtual pure method is added to tdesc_type, which
>> replaces the current tdesc_gdb_type function. Calling this method on a
>> tdesc_type returns the corresponding built gdb type.
>
> Hi Simon,
> Thanks for doing this, patch looks good to me.
>

Thanks, I pushed the series and this patch. I realized too late that I
must have been working on the branch without the fixes after your review
comments... so I added this new patch on top.

Simon

From 858c9d13240e695bc3b750368f5d4e524b12112e Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@ericsson.com>
Date: Tue, 5 Dec 2017 16:39:35 -0500
Subject: [PATCH] Address review comments for the previous series

I failed at git and missed adding/lost changes on the wrong branch, the
result being that I didn't incorporate fixes resulting from Yao's review
comments. This patch fixes that.

There are two places where we should use the unique pointer typedef, and
ChangeLog entries missing.

gdb/ChangeLog:

* target-descriptions.c (struct tdesc_feature) <registers>: Use
tdesc_reg_up typedef.
(struct target_desc) <features>: Use tdesc_feature_up typedef.
---
gdb/ChangeLog | 8 ++++++++
gdb/target-descriptions.c | 4 ++--
2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 1d3a3e8..657f87b 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@
+2017-12-05 Simon Marchi <simon.marchi@ericsson.com>
+
+ * target-descriptions.c (struct tdesc_feature) <registers>: Use
+ tdesc_reg_up typedef.
+ (struct target_desc) <features>: Use tdesc_feature_up typedef.
+
2017-12-05 Simon Marchi <simon.marchi@polymtl.ca>

* target-descriptions.c (struct tdesc_type): Use default
@@ -118,6 +124,8 @@
(tdesc_register_in_reggroup_p): Adjust.
(class print_c_tdesc) <visit>: Adjust.
(class print_c_feature) <visit>: Adjust.
+ * features/arc-arcompact.c: Re-generate.
+ * features/arc-v2.c: Re-generate.

2017-12-05 Simon Marchi <simon.marchi@ericsson.com>

diff --git a/gdb/target-descriptions.c b/gdb/target-descriptions.c
index 12d72fa..5a6f619 100644
--- a/gdb/target-descriptions.c
+++ b/gdb/target-descriptions.c
@@ -529,7 +529,7 @@ struct tdesc_feature : tdesc_element
std::string name;

/* The registers associated with this feature. */
- std::vector<std::unique_ptr<tdesc_reg>> registers;
+ std::vector<tdesc_reg_up> registers;

/* The types associated with this feature. */
std::vector<tdesc_type_up> types;
@@ -613,7 +613,7 @@ struct target_desc : tdesc_element
std::vector<property> properties;

/* The features associated with this target. */
- std::vector<std::unique_ptr<tdesc_feature>> features;
+ std::vector<tdesc_feature_up> features;

void accept (tdesc_element_visitor &v) const override
{
--
2.7.4