[Mesa-dev] [PATCH 2/2] glsl: Rewrote _mesa_glsl_process_extension to use table-driven logic.

[Paul Berry]

On 27 June 2011 18:30, Ian Romanick <idr at freedesktop.org> wrote:
> I like this a lot.  It's a really good clean up of a rotting cesspool.

Thanks!

>>   that are avaiable in geometry shaders.
>
>             available

*smacks forehead*  Oops.

>> +   /* Name of the extension when referred to in a GLSL extension
>> +    * statement */
>
> Field comments should get the doxygen
>
>    /**
>     * Short description
>     *
>     * Detailed description
>     */
>
> or
>
>    /** Description */
>
> treatment.
>
> Also, we generally prefer the closing */ of a multiline comment to be on
> its own line.

Ok, I can do doxygen-style.  BTW, is there a web site where the
doxygen-extracted documentation for mesa is automatically uploaded?
It would be convenient to be able to browse the docs without having to
build them locally.

>> +   /* Flag in the gl_extensions struct indicating whether this
>> +    * extension is supported by the driver, or
>> +    * &gl_extensions::dummy_true if supported by all drivers */
>> +   const GLboolean gl_extensions::* supported_flag;
>
> WTF?  Seriously.  What does this do?  I was expecting to see this code
> use the offsetof macro (like src/mesa/main/extensions.c does), but I'm
> now suspecting that C++ has some built-in magic for this.  Is that true?

Yes.  The feature is called "pointer to data member" and I'm surprised
it doesn't get more press, considering how frequently people reinvent
this particular wheel.  The syntax is pretty straightforward once you
get the hang of it:

- foo bar::* p declares p to be an "offset" to a field of type foo
that exists within struct bar
- &bar::baz computes the "offset" of field baz within struct bar
- x.*p accesses the field of x that exists at "offset" p
- x->*p is equivalent to (*x).*p

I hope my use of this C++ feature doesn't come across as too
newfangled.  IMHO it's superior to the offsetof macro because (a) it
can represent null pointers unambiguously, and (b) the compiler
detects mistakes like referring to a data member of the wrong type, or
referring to a member of the wrong class (both of which would be
uncaught by offsetof).

But it's not always an easy call.  Offsetof, for instance, handles
structures-within-structures and arrays-within-structures more
gracefully than pointers to data members do.

BTW, if you're worried about whether all C++ compilers support this
feature, rest assured that it has been part of the language since the
old CFront days.

>> +const _mesa_glsl_extension _mesa_glsl_supported_extensions[] = {
>
> This should probably be static.

Good point.  I'll fix this.

>> +   case vertex_shader:   if (!this->avail_in_VS) return false; break;
>> +   case geometry_shader: if (!this->avail_in_GS) return false; break;
>> +   case fragment_shader: if (!this->avail_in_FS) return false; break;
>
> Delete the spurious breaks.

Geez, what was I smoking?

>> +   for (unsigned int i = 0; i < Elements(_mesa_glsl_supported_extensions);
>> +        ++i) {
>
> Just 'unsigned'.

Really?  I'm surprised you care about this detail, esp. considering
that there are many instances of "unsigned int" already in Mesa.  But
I'll acquiesce, especially since I'm trying to talk you into letting
me use a little-known C++ feature above :)

>> +            if (extension->compatible_with_state(state)) {
>> +               _mesa_glsl_supported_extensions[i].set_flags(state, behavior);
>
>                  extension->set_flags(state, behavior);

Yes, you're right, of course.

Thanks for your comments, Ian.  I'll post a revised patch later this morning.