[Mesa-dev] [PATCH 01/11] glsl: Initialize all member variables of _mesa_glsl_parse_state on construction.

[Francisco Jerez]

Kenneth Graunke <kenneth at whitecape.org> writes:

> On 09/20/2013 09:51 PM, Francisco Jerez wrote:
>> diff --git a/src/glsl/glsl_parser_extras.cpp b/src/glsl/glsl_parser_extras.cpp
>> index cac5a18..772933f 100644
>> --- a/src/glsl/glsl_parser_extras.cpp
>> +++ b/src/glsl/glsl_parser_extras.cpp
>> @@ -55,7 +55,7 @@ static unsigned known_desktop_glsl_versions[] =
>>  
>>  _mesa_glsl_parse_state::_mesa_glsl_parse_state(struct gl_context *_ctx,
>>  					       GLenum target, void *mem_ctx)
>> - : ctx(_ctx)
>> +   : ctx(_ctx), switch_state()
>
> Maybe it's just me, but I don't particularly like using explicit calls
> to undefined constructors to zero out data.
>
Strictly speaking it's not "undefined" it's an implicitly defined
constructor, just like the implicitly defined destructor, copy
constructor and assignment operators you get according to the C++
standard.  I think this is a very convenient language feature, I'd
rather stick to the convention of assuming that all default constructors
do something reasonable and leave the class in a well-defined state (as
implicitly defined constructors do for POD types), than avoiding calls
to implicitly defined constructors.

> It's a bit of an inconsistency in C++: for most classes, the default
> constructor does nothing.  But for POD, the default constructor
> zero-initializes the data.  Unless you're familiar with this rule, it
> looks like a no-op.
>
For all classes the default constructor calls the default constructors
of all member variables and superclasses recursively, it's never a no-op
unless you explicitly define it and leave all POD fields undefined.

> Perhaps more importantly, the moment someone adds a method to one of
> these classes, the default constructor's behavior changes from
> zero-initializing to no-op.  It's easy to forget that, and suddenly have
> a bunch of uninitialized data.
>
A class having methods defined doesn't change the behavior of the
implicitly defined constructor in any way AFAIK, I really don't see the
problem.

> I would prefer to see explicit constructors for POD types which
> zero-initialize all of the fields.  Then, you don't need to call the
> constructor - it just happens automatically.  If you add methods, it
> keeps working.
>
I agree with doing this for the reason of not having to use parenthesis
to call the constructor on classes with POD members, but again, adding
methods doesn't have any influence on it, and it seems like something
that would be better done as a separate follow-up patch series because
it would involve a considerable amount of changes to do it consistently.

> For tiny structures, like two or three elements, I'd also be okay with
> the containing class's constructor initializing them.  For example,
> _mesa_glsl_parse_state() could just initialize switch_state->test_var
> and so on.  (In this case I think glsl_switch_state is too large for
> this approach.)
>
IMHO having a reasonable default constructor for all classes (possibly
the implicitly generated one) for all classes is usually a more concise
code style and also more robust against changes...

> Sorry for the nitpicking.  Otherwise, I'm definitely in favor of this
> change.
>
Thank you for your comments, I pretty much agree with everything else
you said in this thread, I'll send a revised patch series taking into
account your suggestions soon.

>[...]
-------------- 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/20130924/e2116df3/attachment.pgp>