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

[Matt Turner]

On Wed, Oct 30, 2013 at 8:59 AM, Paul Berry <stereotype441 at gmail.com> wrote:
> 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.)

Excellent, thanks. I'll drop this patch.