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 _______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
