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

[Hans de Goede]

Hi,

On 15-08-16 16:10, walter harms wrote:
>
>
> Am 15.08.2016 09:03, schrieb 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
>>
>>
>
>
> --- a/man/XFree.man
> +++ b/man/XFree.man
> @@ -90,8 +90,9 @@ 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.
> +You should use it to free any objects that were allocated by Xlib, unless
> +an alternate function is explicitly specified for the object.
> +
> +If data is NULL, no operation is performed.
>
>
>
> can we agree on this ?

s/You should use it to free/You must use it to free/ otherwise ack,
bonus points if you leave the ", unless" at the end of the line
as it was originally, this will make the patch smaller.

Regards,

Hans