[Mesa-dev] [PATCH 1/2] gallium/util: check callback pointers for non-null in pipe_debug_message()

[Ilia Mirkin]

On Sat, Dec 5, 2015 at 2:48 PM, Brian Paul <brianp at vmware.com> wrote:
> On 12/05/2015 12:17 PM, Ilia Mirkin wrote:
>>
>> On Sat, Dec 5, 2015 at 2:10 PM, Brian Paul <brianp at vmware.com> wrote:
>>>
>>> So the callers don't have to do it.
>>>
>>> v2: also check cb!=NULL in the macro and move the conditional in
>>> _pipe_debug_message() to enclose all code.
>>> ---
>>>   src/gallium/auxiliary/util/u_debug.c | 9 +++++----
>>>   src/gallium/auxiliary/util/u_debug.h | 8 +++++---
>>>   2 files changed, 10 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/src/gallium/auxiliary/util/u_debug.c
>>> b/src/gallium/auxiliary/util/u_debug.c
>>> index 7029536..2aa75b4 100644
>>> --- a/src/gallium/auxiliary/util/u_debug.c
>>> +++ b/src/gallium/auxiliary/util/u_debug.c
>>> @@ -77,11 +77,12 @@ _pipe_debug_message(
>>>      enum pipe_debug_type type,
>>>      const char *fmt, ...)
>>>   {
>>> -   va_list args;
>>> -   va_start(args, fmt);
>>> -   if (cb && cb->debug_message)
>>> +   if (cb && cb->debug_message) {
>>
>>
>> Shouldn't this if () just have been removed?
>
>
> I kept it in case the function were called directly (maybe because a
> non-default ID was desired).
>
>
>> Separately, I've been
>> told that the va_* "functions" shouldn't be inside of control flow,
>> since they're often funky macros, although it does seem like this
>> ought to work even in that scenario.
>
>
> I wasn't aware of that.  I can undo that part of the patch if you prefer.
>
>
>>
>> Either way, this patch is Reviewed-by: Ilia Mirkin
>> <imirkin at alum.mit.edu> but with a minor preference to removing the
>> check entirely here now that it's in the macro.
>
>
> Do you ever expect someone to call the function directly w/out the macro
> wrapper?

No, but I see what you're saying. This is all so insignificant, so do
whatever you think is best.

Cheers,

  -ilia