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

[Chad Versace]

On 03/16/2012 01:48 PM, Paul Berry wrote:
> On 16 March 2012 11:46, Chad Versace <chad.versace at linux.intel.com <mailto: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 --
> 
>     > +
>     > +/**
>     > + * 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".

I prefer @param, but in Piglit I use \param because that seemed to be the convention. I don't care either way, though.
 
>     > +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.

Thanks for making the change. I do like naming functions "reset" rather than "init" when they can be called more than once.


> 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.

Looking forward to it.

----
Chad Versace
chad.versace at linux.intel.com