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

[Eric Anholt]

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.

> 
> >> +   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. :)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20110628/b4aab4fb/attachment-0001.pgp>