Re: [PATCH 5/6] Use copy ctor in regcache_dup

[Pedro Alves <palves@redhat.com>]

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