[5/6] Use copy ctor in regcache_dup

  gdb:

2017-04-25  Yao Qi  <yao.qi@linaro.org>

	* regcache.c (regcache::regcache): New copy ctor.
	(do_cooked_read): Moved above.
	(regcache_dup): Use copy ctor.
	* regcache.h (struct regcache): Declare copy ctor and remove
	friend regcache_dup.
---
 gdb/regcache.c | 30 ++++++++++++++++--------------
 gdb/regcache.h |  7 ++++---
 2 files changed, 20 insertions(+), 17 deletions(-)
  

On 04/25/2017 09:28 PM, Yao Qi wrote:
> gdb:
> 
> 2017-04-25  Yao Qi  <yao.qi@linaro.org>
> 
> 	* regcache.c (regcache::regcache): New copy ctor.
> 	(do_cooked_read): Moved above.
> 	(regcache_dup): Use copy ctor.
> 	* regcache.h (struct regcache): Declare copy ctor and remove
> 	friend regcache_dup.
> ---

> index e6494a9..0d302a7 100644
> --- a/gdb/regcache.h
> +++ b/gdb/regcache.h
> @@ -240,6 +240,10 @@ public:
>      : regcache (gdbarch, aspace_, true)
>    {}
>  
> +  /* Copy constructor, create a readonly regcache from a non-readonly
> +     regcache.  */
> +  regcache (const regcache &);

This one doesn't look right to me.  This isn't a copy in the
normal C++ object copy sense.  The new object isn't semantically the
same as the source.  One can't use the new object the same way as the
source regcache, they're not interchangeable.  This is bound to generate
confusion and problems.

Considering patch #6, it'd make more sense to me to
make that a separate constructor with tag dispatching, like:

struct regcache
{
  struct readonly_t {};
  static constexpr readonly_t readonly {};

  regcache (readonly_t, const regcache &src); // old regcache_dup
};

Then used like:

regcache ro_copy (regcache::readonly, src);

or if you want, you could make that tag-based ctor private and
add a factory function:

struct regcache
{
private:
  struct readonly_t {};
  regcache(readonly_t, const regcache &src);

  regcache(regcache &&src) { // implement this } // move ctor

public:
  static regcache make_readonly_copy (const regcache &src)
  {
    return regcache (readonly_t{}, src);
  }
};

Used like 

 regcache ro_copy = regcache::make_readonly_copy (src);

In any case, I think we should make sure to disable
the regular copy methods since the type doesn't really
support normal copy:

  regcache(const regcache &) = delete;
  void operator= (const regcache &) = delete;

Thanks,
Pedro Alves
  

Pedro Alves <palves@redhat.com> writes:

> This one doesn't look right to me.  This isn't a copy in the
> normal C++ object copy sense.  The new object isn't semantically the
> same as the source.  One can't use the new object the same way as the
> source regcache, they're not interchangeable.  This is bound to generate
> confusion and problems.

I thought about this.  The reason I still do this is that I can't think
of a case that we need to copy a read-write regcache to another
read-write regcache.  So far, we only use copy(or transform) a
read-write regcache to a read-only regcache.  However, I agree with you,
it is not a normal "copy ctor".

>
> Considering patch #6, it'd make more sense to me to
> make that a separate constructor with tag dispatching, like:
>
> struct regcache
> {
>   struct readonly_t {};
>   static constexpr readonly_t readonly {};
>
>   regcache (readonly_t, const regcache &src); // old regcache_dup
> };
>
> Then used like:
>
> regcache ro_copy (regcache::readonly, src);
>
> or if you want, you could make that tag-based ctor private and
> add a factory function:
>
> struct regcache
> {
> private:
>   struct readonly_t {};
>   regcache(readonly_t, const regcache &src);
>
>   regcache(regcache &&src) { // implement this } // move ctor
>
> public:
>   static regcache make_readonly_copy (const regcache &src)
>   {
>     return regcache (readonly_t{}, src);
>   }
> };
>
> Used like 
>
>  regcache ro_copy = regcache::make_readonly_copy (src);

I have a different design on this, that is, put readonly regcache and
readwrite regcache to two classes.  readwrite regcache inherits readonly
regcache, and readonly regcache has a constructor whose argument is a
readwrite regcache.

class readonly_regcache
{
public:
  explicit readonly_regcache (const regcache &);
}

class regcache : public readonly_regcache
{
}

What do you think?

>
> In any case, I think we should make sure to disable
> the regular copy methods since the type doesn't really
> support normal copy:
>
>   regcache(const regcache &) = delete;
>   void operator= (const regcache &) = delete;

I agree.  I'll add it.
  

On 04/28/2017 10:11 AM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
>> This one doesn't look right to me.  This isn't a copy in the
>> normal C++ object copy sense.  The new object isn't semantically the
>> same as the source.  One can't use the new object the same way as the
>> source regcache, they're not interchangeable.  This is bound to generate
>> confusion and problems.
> 
> I thought about this.  The reason I still do this is that I can't think
> of a case that we need to copy a read-write regcache to another
> read-write regcache.  So far, we only use copy(or transform) a
> read-write regcache to a read-only regcache.  However, I agree with you,
> it is not a normal "copy ctor".
> 
>>
>> Considering patch #6, it'd make more sense to me to
>> make that a separate constructor with tag dispatching, like:
>>
>> struct regcache
>> {
>>   struct readonly_t {};
>>   static constexpr readonly_t readonly {};
>>
>>   regcache (readonly_t, const regcache &src); // old regcache_dup
>> };
>>
>> Then used like:
>>
>> regcache ro_copy (regcache::readonly, src);
>>
>> or if you want, you could make that tag-based ctor private and
>> add a factory function:
>>


> 
> I have a different design on this, that is, put readonly regcache and
> readwrite regcache to two classes.  readwrite regcache inherits readonly
> regcache, and readonly regcache has a constructor whose argument is a
> readwrite regcache.

*nod*  I had seen it in the series' intro, but was going with the
idea of avoid that work.  If we can get it nicely expressed in
the type system, then it's even better.

> 
> class readonly_regcache
> {
> public:
>   explicit readonly_regcache (const regcache &);
> }
> 
> class regcache : public readonly_regcache
> {
> }
> 
> What do you think?

I think that that's too incomplete to evaluate.  :-)

Why regcache on top of readonly_regcache and not the other
way around?  Off hand, I'd think that 

 struct regcache_base;
 struct readonly_regcache : regcache_base {};
 struct regcache : regcache_base {};

would be the "obvious" first choice.

What happens to all the "regcache->readonly_p" checks in
spread around in multiple functions?  Do they disappear?  
I think that if the design ends up with that flag still present 
and functions exposing interfaces that work with
a "struct regcache *" that can either be readonly
or write-through, then it's likely that the design
isn't complete.  [But replacing a single boolean
checked in a few select places by virtual methods and a
full blown vtable and a bunch of dispatching makes me
cringe a bit too.  :-)  But OTOH, I suspect you want
to add virtual methods for unit testing.]

I'm totally not against this direction, to be clear,
but it'd still suggest adding the tag dispatch ctor
first (the simple version with no factory, just adds two
lines of code compared to the copy ctor version), which
allows getting rid of the heap allocation and the cleanups
as you're doing in patch #6, and then consider changing
the hierarchy in a follow up patch.

> 
>>
>> In any case, I think we should make sure to disable
>> the regular copy methods since the type doesn't really
>> support normal copy:
>>
>>   regcache(const regcache &) = delete;
>>   void operator= (const regcache &) = delete;
> 
> I agree.  I'll add it.

Thanks.
  

Pedro Alves <palves@redhat.com> writes:

> I think that that's too incomplete to evaluate.  :-)
>
> Why regcache on top of readonly_regcache and not the other
> way around?  Off hand, I'd think that 
>
>  struct regcache_base;
>  struct readonly_regcache : regcache_base {};
>  struct regcache : regcache_base {};
>
> would be the "obvious" first choice.

I have to agree, design like this affects other parts a lot though.

>
> What happens to all the "regcache->readonly_p" checks in
> spread around in multiple functions?  Do they disappear?

I wish they will.
  
> I think that if the design ends up with that flag still present 
> and functions exposing interfaces that work with
> a "struct regcache *" that can either be readonly
> or write-through, then it's likely that the design
> isn't complete.  [But replacing a single boolean
> checked in a few select places by virtual methods and a
> full blown vtable and a bunch of dispatching makes me
> cringe a bit too.  :-)  But OTOH, I suspect you want
> to add virtual methods for unit testing.]

I get your point and let me see how far can I go.  Yes, I need to make
some regcache methods virtual for unit tests, so far I only need to make
raw_write and ~regcache virtual.

>
> I'm totally not against this direction, to be clear,
> but it'd still suggest adding the tag dispatch ctor
> first (the simple version with no factory, just adds two
> lines of code compared to the copy ctor version), which
> allows getting rid of the heap allocation and the cleanups
> as you're doing in patch #6, and then consider changing
> the hierarchy in a follow up patch.

No problem, I'll replace my copy ctor with your tag dispatch ctor.