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

[Ian Romanick]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 06/28/2011 10:01 AM, Eric Anholt wrote:
> On Tue, 28 Jun 2011 06:49:57 -0700, Paul Berry <stereotype441 at gmail.com> wrote:
>> 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).
> 
> If I stumbled on this code, I'd have no idea what was going on.  A short
> version of this description near the code might help us poor C
> developers who stumble on it that have never seen this stuff before.
> 
>>>> +   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?
> 
> They don't look spurious in the code quoted here.  But they do look like
> some more whitespace would help a lot.

Oops.  mod + 1.  *blush*

>>>> +   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 :)
> 
> I personally always preferred unsigned int to unsigned. :)
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAk4KPlYACgkQX1gOwKyEAw/g7gCaAvNB5BkfrzvgPNM9x8v5+dA/
gywAoIBeM9cEvNiDaj+XvEk9vdOcU4gV
=Glg9
-----END PGP SIGNATURE-----