[Mesa-dev] [PATCH 08/15] i965/fs: Add fs_inst copy constructor.

[Paul Berry]

On 29 October 2013 17:23, Matt Turner <mattst88 at gmail.com> wrote:

> On Tue, Oct 29, 2013 at 2:32 AM, Pohjolainen, Topi
> <topi.pohjolainen at intel.com> wrote:
> > On Mon, Oct 28, 2013 at 11:31:32AM -0700, Matt Turner wrote:
> >> ---
> >>  src/mesa/drivers/dri/i965/brw_fs.cpp | 30
> ++++++++++++++++++++++++++++++
> >>  src/mesa/drivers/dri/i965/brw_fs.h   |  1 +
> >>  2 files changed, 31 insertions(+)
> >>
> >> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp
> b/src/mesa/drivers/dri/i965/brw_fs.cpp
> >> index b2eac6c..28d369a 100644
> >> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
> >> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
> >> @@ -71,6 +71,36 @@ fs_inst::fs_inst()
> >>     init();
> >>  }
> >>
> >> +fs_inst::fs_inst(const fs_inst *that)
> >
> > Is there a particular reason why you chose to introduce this as a
> conversion
> > constructor instead of as a proper copy? I'm just afraid that it allows
> the
> > compiler to accept, for example, the following (which is probably not
> what the
> > author wanted):
>
> No, in fact I really dislike this code. Is it safe to just memcpy the
> object in a copy constructor? Having to maintain the list of fields is
> awful.
>
> I'm happy to receive suggestions.
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>

The compiler automatically generates a default copy constructor that
effectively does a memcpy*, but it has the signature:

    fs_inst::fs_inst(const fs_inst &)

instead of

    fs_inst::fs_inst(const fs_inst *)

So you can just drop this patch, and in patch 9/15 change

    cmp_inst = new(mem_ctx) fs_inst(m);

to:

    cmp_inst = new(mem_ctx) fs_inst(*m);

(*actually the default copy constructor is smarter than a memcpy.  It
memcpy's all the fields of the class that are safe to memcpy (those that
don't have custom copy constructors), and calls the custom copy
constructors on the rest.)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20131030/411d22f3/attachment.html>