On Tue, 28 Jun 2011 06:49:57 -0700, Paul Berry <stereotype...@gmail.com> wrote: > On 27 June 2011 18:30, Ian Romanick <i...@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. :)
pgpjQ2j6PTOw4.pgp
Description: PGP signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
