On 09/15/2017 07:42 AM, Emil Velikov wrote:
Hi Kyle,
On 8 September 2017 at 13:09, Emil Velikov <[email protected]> wrote:
On 7 September 2017 at 19:21, Kyle Brenneman <[email protected]> wrote:
On 09/07/2017 11:56 AM, Emil Velikov wrote:
On 7 September 2017 at 18:36, Kyle Brenneman <[email protected]>
wrote:
On 09/07/2017 10:03 AM, Emil Velikov wrote:
From: Emil Velikov <[email protected]>
Instead of having three, almost identical but not quite,
_eglDebugReport* functions, simply fold them into one.
While doing so drop the unnecessary arguments 'command' and
'objectLabel'. Former is identical to funcName, while the latter is
already stored (yet unused) in _EGLThreadInfo::CurrentObjectLabel.
Cc: Kyle Brenneman <[email protected]>
Cc: Adam Jackson <[email protected]>
Signed-off-by: Emil Velikov <[email protected]>
---
src/egl/main/eglapi.c | 3 +--
src/egl/main/eglcurrent.c | 53
++++++++++++-----------------------------------
src/egl/main/eglcurrent.h | 7 -------
3 files changed, 14 insertions(+), 49 deletions(-)
The command and funcName parameters aren't always identical. funcName is
whatever string got passed to _eglError, which is often the name of some
internal function. For instance, eglCreateWindowSurface can call
_eglError
with the string "_eglCreateWindowSurfaceCommon". A lot of the calls in
the
src/egl/drivers/ directory also use internal function names.
I had both the command and fullName parameters in order to preserve the
existing error reporting behavior (in _eglInternalError) in addition to
the
debug callback.
Also, the reason I kept the command parameter at all instead of always
using
thr->CurrentFuncName is so that _eglSetFuncName could correctly report an
EGL_BAD_ALLOC error if it couldn't allocate the _EGLThreadInfo struct in
the
first place. In that case, (thr->CurrentFuncName) would be NULL.
You could change _eglError to pass NULL for the command name, in which
case
the debug callback would get the correct name in all cases, but then
you'd
lose the internal function name that would otherwise get passed to
_eglLog.
I agree on the _eglError front - it is a bit iffy, as described in 3/4
[1].
At the same time I cannot find an instance where there will be a
difference.
Can you point me to one?
Ah, I hadn't read patch 3/4 yet when I typed my reply.
Without patch 3, if the app calls eglCreateWindowSurface with NULL for the
native window handle, then you'd run into that. eglCreateWindowSurface calls
_eglCreateWindowSurfaceCommon, which calls the RETURN_EGL_ERROR macro if it
hits an error, which in turn would pass the string
"eglCreateWindowSurfaceCommon" to _eglError, and from there to the callback.
Changing _eglError like you did in patch 3 would avoid that problem.
Right, my bad. I'll swap the patch order.
However, I'm not sure how useful an internal function name by itself would
be for an application developer, which is generally who the debug messages
are aimed at.
Here is where the wonder of open-source comes handy :-P
The message makes it easier to track and resolve the problem... say
you cannot report the issue (for whatever reason).
Overall I agree though, messages kind of suck. Yet again that's seems
orthogonal to what the series does.
Also, if you do use the msg parameter in eglError as the debug callback
message, then it should probably use (..., "%s", msg), on the off-chance
that some message ever has a printf character in it.
As/if we're to beat sense into _eglError "%s" would be very useful.
ATM nobody misuses the argument so we are safe.
With the above in mind, can you suggest any changes that I should make
to the patch?
Alternatively can you please review/ack the patch, please.
Thanks
Emil
If patch 3 gets moved to the beginning, then the set looks correct to
me. I'd still recommend changing it to use a "%s" while you're at it,
but I'll leave that to your discretion. It's not immediately needed, but
it's an easy change and one less thing to go wrong in the future.
-Kyle
_______________________________________________
mesa-dev mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
