[PATCH libXrandr 1/2] fix: doGetScreenResources() info: redundant null check on calling free()

[Hans de Goede]

Hi,

On 13-08-16 19:53, Hans de Goede wrote:
> Hi,
>
> On 12-08-16 18:10, walter harms wrote:
>>
>>
>> Am 12.08.2016 16:36, schrieb Hans de Goede:
>>> Hi,
>>>
>>> On 12-08-16 16:09, walter harms wrote:
>>>>
>>>>
>>>> Am 12.08.2016 12:11, schrieb Hans de Goede:
>>>>> Hi,
>>>>>
>>>>> On 28-07-16 19:31, walter harms wrote:
>>>>>>
>>>>>> janitorial patch: remove some unneeded if() before free()
>>>>>
>>>>> This is not free() but Xfree() and "man Xfree" states:
>>>>>
>>>>> "A NULL pointer cannot be passed to this function."
>>>>>
>>>>
>>>> this is wrong.
>>>>
>>>> Xfree is a define for free()
>>>> Xlibint.h:#define Xfree(ptr) free((ptr))
>>>
>>> Ah, yes then the man-page is wrong, my bad.
>>>
>>>> more over the general use is this way.
>>>>
>>>> I will post a patch for the man-page.
>>>>
>>>> do you thing this is understandable ?
>>>
>>> Maybe explicitly state that passing NULL
>>> is allowed and will do nothing ?
>>>
>>> e.g. man 3 free has:
>>>
>>> "If ptr is NULL, no operation is performed."
>>>
>>> Regards,
>>>
>>
>> I did some emphasis on free since if someone has
>> a non-posix free() it would be a problem,
>
> Looks good to me, can you submit this as a proper patch please ?

Scrap that I just noticed the "It is a alternativ for free(3)." language
you added as well as changing must into should. Please do not do that
people must really always use XFree to free Xlib allocated resources.

Mixing / matching different alloc and free functions can end badly
if the backend of one of them changes.

In this light I also suggest that you drop the reference to free(3) in
the second sentence just make it. "If data is NULL, no operation is performed."

Regards,

Hans






>
> Thanks and Regards,
>
> Hans
>
>
>>
>> --- a/man/XFree.man
>> +++ b/man/XFree.man
>> @@ -90,8 +90,10 @@ Specifies the data that are to be freed.
>>  The
>>  .ZN XFree
>>  function is a general-purpose Xlib routine that frees the specified data.
>> -You must use it to free any objects that were allocated by Xlib,
>> -unless an alternate function is explicitly specified for the object.
>> -A NULL pointer cannot be passed to this function.
>> +It is a alternativ for free(3). You should use it to free any objects that
>> +were allocated by Xlib, unless an alternate function is explicitly specified
>> +for the object.
>> +
>> +Since it is based on free(3) if data is NULL, no operation is performed.
>>  .SH "SEE ALSO"
>>  \fI\*(xL\fP
>>
>>
>>
>>> Hans
>>>
>>>
>>>
>>>>
>>>> --- a/man/XFree.man
>>>> +++ b/man/XFree.man
>>>> @@ -90,8 +90,8 @@ Specifies the data that are to be freed.
>>>>  The
>>>>  .ZN XFree
>>>>  function is a general-purpose Xlib routine that frees the specified
>>>> data.
>>>> -You must use it to free any objects that were allocated by Xlib,
>>>> -unless an alternate function is explicitly specified for the object.
>>>> -A NULL pointer cannot be passed to this function.
>>>> +It is a alternativ for free(3). You should use it to free any objects
>>>> that
>>>> +were allocated by Xlib, unless an alternate function is explicitly
>>>> specified
>>>> +for the object.
>>>>  .SH "SEE ALSO"
>>>>  \fI\*(xL\fP
>>>>
>>>>
>>>>
>>>>> Regards,
>>>>>
>>>>> Hans
>>>>>
>>>>>
>>>>>>
>>>>>>
>>>>>> ---
>>>>>>  src/XrrScreen.c | 4 ++--
>>>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/src/XrrScreen.c b/src/XrrScreen.c
>>>>>> index f29071c..b8ce7e5 100644
>>>>>> --- a/src/XrrScreen.c
>>>>>> +++ b/src/XrrScreen.c
>>>>>> @@ -127,8 +127,8 @@ doGetScreenResources (Display *dpy, Window window,
>>>>>> int poll)
>>>>>>      xrsr = (XRRScreenResources *) Xmalloc(rbytes);
>>>>>>      wire_names = (char *) Xmalloc (rep.nbytesNames);
>>>>>>      if (xrsr == NULL || wire_names == NULL) {
>>>>>> -    if (xrsr) Xfree (xrsr);
>>>>>> -    if (wire_names) Xfree (wire_names);
>>>>>> +    Xfree (xrsr);
>>>>>> +    Xfree (wire_names);
>>>>>>      _XEatDataWords (dpy, rep.length);
>>>>>>      UnlockDisplay (dpy);
>>>>>>      SyncHandle ();
>>>>>>
>> _______________________________________________
>> xorg-devel at lists.x.org: X.Org development
>> Archives: http://lists.x.org/archives/xorg-devel
>> Info: https://lists.x.org/mailman/listinfo/xorg-devel
>>