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

[Simon Marchi <simon.marchi@ericsson.com>]

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