[Piglit] [PATCH v2 09/13] piglit-dispatch: Remaining infrastructure.
Chad Versace
chad.versace at linux.intel.com
Mon Mar 19 09:49:04 PDT 2012
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
More information about the Piglit
mailing list