[Piglit] [PATCH v2 09/13] piglit-dispatch: Remaining infrastructure.

[Paul Berry]

On 16 March 2012 11:46, Chad Versace <chad.versace at linux.intel.com> wrote:

> Overall this patch looks good. Below are some comments, some more important
> than others...
>
> On 03/12/2012 02:41 PM, Paul Berry wrote:
> > Previous patches introduced code generation for the auto-generated
> > portions of the piglit-dispatch mechanism.  This patch supplies the
> > remaining hand-written code, which includes:
> >
> > - Code to initialize piglit-dispatch.
> >
> > - The actions to be taken when looking up a function, when function
> >   lookup fails, or when a function is unsupported.
> >
> > - Code to determine whether piglit-dispatch has been initialized,
> >   which GL version is supported, and whether a given extension is
> >   supported.
> >
> > - Code to look up a function using a name supplied at run-time.
> >
> > - Definitions of basic GL types.
> > ---
> >  tests/util/piglit-dispatch-init.c |  122 +++++++++++++++++++
> >  tests/util/piglit-dispatch.c      |  238
> +++++++++++++++++++++++++++++++++++++
> >  tests/util/piglit-dispatch.h      |  149 +++++++++++++++++++++++
> >  3 files changed, 509 insertions(+), 0 deletions(-)
> >  create mode 100644 tests/util/piglit-dispatch-init.c
> >  create mode 100644 tests/util/piglit-dispatch.c
> >  create mode 100644 tests/util/piglit-dispatch.h
>
> -- snip --
>
> > --- /dev/null
> > +++ b/tests/util/piglit-dispatch.c
> > @@ -0,0 +1,238 @@
> > +/* Copyright 2012 Intel Corporation
> > + *
> > + * Permission is hereby granted, free of charge, to any person
> obtaining a
> > + * copy of this software and associated documentation files (the
> "Software"),
> > + * to deal in the Software without restriction, including without
> limitation
> > + * the rights to use, copy, modify, merge, publish, distribute,
> sublicense,
> > + * and/or sell copies of the Software, and to permit persons to whom the
> > + * Software is furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice (including the
> next
> > + * paragraph) shall be included in all copies or substantial portions
> of the
> > + * Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT
> SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
> OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> ARISING
> > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> DEALINGS
> > + * IN THE SOFTWARE.
> > + */
> > +
> > +#include "piglit-util.h"
>
> I think you need to #include "piglit-dispatch.h" too.
>

Huh, I wonder why this ever worked.

Oh, I see: piglit-util.h includes gl_wrap.h, which includes
piglit-dispatch.h.  Your suggestion is way clearer, and there's no harm in
having a redundant include.  I'll include piglit-dispatch.h before
piglit-util.h.


>
> > +/* Global state maintained by the Piglit dispatch mechanism: */
> > +
> > +/**
> > + * Which function to call to get the address of a core function.
> > + */
> > +static piglit_get_proc_address_function_ptr __get_core_proc_address =
> NULL;
>
> Identifiers that begin with __ are reserved, even if the identifier has
> static
> or local scope. They need to be renamed.
>
> From C in a Nutshell, O'Reilly, Ch 15, "Reserved Identifiers":
>  All identifiers that begin with an underscore, followed by a second under-
>  score or an uppercase letter, are always reserved. Thus you cannot use
> identi-
>  fiers such as _ _x or _Max, even for local variables or labels.
>

Oh, bother.  I did this because I was imitating GLEW, but you're right--I
really should leave "__" names for compilers to use for their own
purposes.  Fortunately there's not really any danger of conflicting names
anyhow, since everything that isn't static has a name that starts with
"piglit_dispatch_".  I'll drop all the double underscores.


>
> -- snip --
>
> > +
> > +/**
> > + * Initialize the dispatch mechanism.
> > + *
> > + * - api is the API under test.  This determines whether deprecated
> > + *   functionality is supported (since deprecated functions cannot be
> > + *   used in forward compatible contexts).  It also affects which GL
> > + *   version is queried for (since, for example a function might be
> > + *   supported in GLES as of version 2.0, but in OpenGL only as of
> > + *   version 2.1).  Not yet implemented.
> > + *
> > + * - get_core_proc and get_ext_proc are the functions to call to
> > + *   retrieve the address of a core GL function or an extension
> > + *   function.  Note that for OpenGL, these can both map to the same
> > + *   function (e.g. glXGetProcAddressARB).  However, in GLES, core
> > + *   functions are not allowed to be queried using the GetProcAddress
> > + *   mechanism, so get_core_proc will need to be implemented by
> > + *   looking up a symbol in a shared library (e.g. using dlsym()).
> > + *
> > + * - unsupported_proc is the function to call if a test attempts to
> > + *   use unsupported GL functionality.  It is passed the name of the
> > + *   function that the test attempted to use.
> > + *
> > + * - failure_proc is the function to call if a call to get_core_proc()
> > + *   or get_ext_proc() unexpectedly returned NULL.  It is passed the
> > + *   name of the function that was passed to get_core_proc() or
> > + *   get_ext_proc().
> > + */
>
> Since we're using Doxygen-style comments, let's use Doxygen's @param tag.
>

What's your opinion on "@param" vs "\param"?  Piglit doesn't use either one
very frequently, but it uses "\param" a lot more than "@param".


>
> > +void
> > +piglit_dispatch_init(piglit_dispatch_api api,
> > +                  piglit_get_proc_address_function_ptr get_core_proc,
> > +                  piglit_get_proc_address_function_ptr get_ext_proc,
> > +                  piglit_error_function_ptr unsupported_proc,
> > +                  piglit_error_function_ptr failure_proc)
> > +{
> > +     (void) api; /* Not yet implemented--assume GL. */
> > +
> > +     __get_core_proc_address = get_core_proc;
> > +     __get_ext_proc_address = get_ext_proc;
> > +     __unsupported = unsupported_proc;
> > +     __get_proc_address_failure = failure_proc;
> > +
> > +     initialize_dispatch_pointers();
>
> initialize_dispatch_pointers() does a lot of work, and that work is
> redundant
> if this is the first call to piglit_dispatch_init(). I would prefer if we
> called initialize_dispatch_pointers() only if the pointer state is dirty;
> that is, if this is not the first call to piglit_dispatch_init().
>
> This is just my preference, not a hard request.
>

Ok, I can go along with that.  initialize_dispatch_pointers() is kind of a
pointless function right now anyhow, since at the moment
piglit_dispatch_init() is only ever called once per process.  But I suspect
that won't always be the case.

I'm also going to rename initialize_dispatch_pointers() to
reset_dispatch_pointers() to make it more obvious why it doesn't need to be
called the first time through.


I'm in the process of preparing v3 of the patch series (which works on
Windows, and hopefully that means it will be the final version).  So expect
to see a revised patch Monday or Tuesday.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/piglit/attachments/20120316/cad55ab2/attachment.htm>