[Mesa-dev] [PATCH 1/2] i965: Allocate vec4_visitor's uniform_size and uniform_vector_size arrays dynamically.

[Francisco Jerez]

Ian Romanick <idr at freedesktop.org> writes:

> On 11/22/2013 12:09 AM, Petri Latvala wrote:
>[...]
>> @@ -546,6 +548,12 @@ private:
>>      * If true, then register allocation should fail instead of spilling.
>>      */
>>     const bool no_spills;
>> +
>> +   /*
>> +    * Make noncopyable.
>> +    */
>> +   vec4_visitor(const vec4_visitor&);
>> +   vec4_visitor& operator=(const vec4_visitor&);
>
> I think this should be a separate patch with it's own justification.
> I'm also not sure it's strictly necessary.  I'm not a C++ expert (and
> most people on the project aren't either), so bear with me a bit.  The
> usual reason to make a class non-copyable is because the default
> copy-constructor will only make a shallow copy of pointer fields.  This
> can lead to either double-frees or dereferencing freed memory.  However,
> since we're using ralloc, and the first construction of a vec4_visitor
> object will out-live any possible copies, neither of these situations
> can occur.  Is that a fair assessment?
>
> Now... we probably should do this anyway... and there are a lot of
> classes in the i965 back-end where this should occur.  I don't know if
> we want to make a macro to generate this boiler-plate code or if we just
> want to manually add it to every class.  Perhaps Ken, Paul, or Curro
> will have an opinion...
>

This hunk looks perfectly fine to me (well, aside from the &-placement).
All singleton classes should have its copy-constructor declared private
to avoid the situations you have described, and adding a member variable
that needs special handling on copy construction seems like the right
time to do so.  I'm OK with using the private copy-constructor and
assignment operator idiom explicitly as in Petri's code or with having a
macro for the same purpose to save typing -- A third possibility that I
find slightly more elegant would be to define a noncopyable class all
singleton objects could inherit from, like:

| class noncopyable {
|    private:
|       noncopyable(noncopyable &);
|       noncopyable &operator=(const noncopyable &);
| };

Because macros that declare class members and need to use access
specifiers can have unexpected side-effects.

Thanks.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 229 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20131122/e507e7a4/attachment.pgp>