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

[Yao Qi <qiyaoltc@gmail.com>]

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.

-- 
Yao (齐尧)