[Mesa-dev] [PATCH 02/15] mesa: Share common code between ARB_debug_output and KHR_debug functions

[Ian Romanick]

On 09/04/2013 04:46 PM, Timothy Arceri wrote:
>> Since the calling functions all should alias, all of this should
>> be removed too.
> 
> The functions have to do different validations on types they are not 
> exactly the same. If there is some way to signal this to be done 
> using an alias can you please give me some advice on how to do this 
> so I can learn.

The general guideline is that if an extension (or core GL version) only
adds functionality, the functions should alias.  In other words, alias
if the new thing is a proper superset of the old thing.

In this case, all of the functionality in the ARB_debug_output is
unchanged in KHR_debug.  Both

    glDebugMessageControl(GL_DEBUG_SOURCE_THIRD_PARTY,
                          GL_DONT_CARE, GL_DONT_CARE, 0,
                          NULL, GL_FALSE);

and

    glDebugMessageControlARB(GL_DEBUG_SOURCE_THIRD_PARTY_ARB,
                             GL_DONT_CARE, GL_DONT_CARE, 0,
                             NULL, GL_FALSE);

behave identically.

In addition, notice that GL_DEBUG_CALLBACK_FUNCTION_ARB is the same as
GL_DEBUG_CALLBACK_FUNCTION, and GL_DEBUG_CALLBACK_USER_PARAM_ARB is the
same as GL_DEBUG_CALLBACK_USER_PARAM.  If glDebugMessageCallbackARB and
glDebugMessageCallback are different, which state does
glGetPointerv(GL_DEBUG_CALLBACK_FUNCTION) return?

It used to be easier in the days when specs still defined GLX protocol.
 If two functions had the same protocol opcode assigned, they had to alias.

We (I) have made mistakes the other direction in the past.  In Mesa 9.1
and earlier, glBindFramebufferEXT and glBindFramebuffer alias, but they
should not.

Let's be cautious... let's poke at the AMD and NVIDIA drivers to see
what they do.  It should be easy enough to figure out if they use the
same implementation for the KHR and ARB functions.

There's a Khronos face-to-face next week, and I'll raise this as a bug.
 KHR_debug really should have listed interactions with ARB_debug.

>> I think this entire patch should be removed.
> 
> I'm trying to stay positive be its hard when your emails and review
> have an obvious overtone of annoyance.

Several people reviewed the patch series, and yet a bunch of obvious
problems slipped through.  *That* is very annoying.

> The entire patch is not junk. For example the changes to the

Well... I don't think I said "junk."  I don't think there's anything in
the series that is junk, per se.  There are a bunch of little things
that should have been noticed and fixed before the code landed.

> validation function are still valid as mentioned above however you
> want to call them, and the generic message_insert function is reused
> by the push/pop implementation. If answer my question from above I
> can create a new patch.

To me, this sounds like the patch does a bunch of different things...
and it should have been split into multiple patches.  Can I add that to
the list of things the reviewers didn't catch? :)