[vlc-devel] [PATCH 2/3] queue: helpers for killable queues

[Steve Lhomme]

On 2020-04-14 9:37, Rémi Denis-Courmont wrote:
> Le tiistaina 14. huhtikuuta 2020, 8.43.24 EEST Steve Lhomme a écrit :
>> On 2020-04-11 21:12, Rémi Denis-Courmont wrote:
>>> This adds two functions to simplify the common case of a thread waiting on
>>> a queue until it is terminated.
>>> ---
>>>
>>>    include/vlc_queue.h | 42 ++++++++++++++++++++++++++++++++++++++++++
>>>    1 file changed, 42 insertions(+)
>>>
>>> diff --git a/include/vlc_queue.h b/include/vlc_queue.h
>>> index eed69510dc..f41d72fb55 100644
>>> --- a/include/vlc_queue.h
>>> +++ b/include/vlc_queue.h
>>> @@ -214,5 +214,47 @@ VLC_USED;
>>>
>>>     */
>>>    
>>>    VLC_API void *vlc_queue_DequeueAll(vlc_queue_t *) VLC_USED;
>>>
>>> +/**
>>> + * @defgroup queue_killable Killable queues
>>> + *
>>> + * Thread-safe queues with an end flag.
>>> + *
>>> + * @{
>>> + */
>>> +
>>> +/**
>>> + * Marks a queue ended.
>>> + */
>>> +static inline void vlc_queue_Kill(vlc_queue_t *q,
>>> +                                  bool *restrict tombstone)
>>> +{
>>> +    vlc_queue_Lock(q);
>>> +    *tombstone = true;
>>
>> What's the point of returning a bool if it's always the same value. And
>> why not return it directly rather than using a pointer ?
> 
> There would be no point returning a constant indeed. And the code doesn't.

I had to read the surrounding code to understand what this code. This 
function should at least have more documentation. The mnemonic 
"tombstone" is not self-explanatory.

If you wanted some helpers it would be nicer with a "killable" queue and 
put the dead signal in the killable queue structure.

If all queues are assumed to be killable then the flag should be in the 
structure directly.

>>> +    vlc_queue_Signal(q);
>>> +    vlc_queue_Unlock(q);
>>> +}
>>> +
>>> +/**
>>> + * Dequeues one entry from a killable queue.
>>> + *
>>> + * @return an entry, or NULL if the queue is empty and has been ended.
>>> + */
>>> +static inline void *vlc_queue_DequeueKillable(vlc_queue_t *q,
>>> +                                              ptrdiff_t offset,
>>> +                                              bool *restrict tombstone)
>>> +{
>>> +    void *entry;
>>> +
>>> +    vlc_queue_Lock(q);
>>> +    while (vlc_queue_IsEmpty(q) && !*tombstone)
>>
>> I would put the bool test in a surrounding if().
> 
> That's not how condition variables work.
> 
>>> +        vlc_queue_Wait(q);
>>> +
>>> +    entry = vlc_queue_DequeueUnlocked(q, offset);
>>> +    vlc_queue_Unlock(q);
>>> +    return entry;
>>> +}
>>> +
>>> +/** @} */
>>> +
>>>
>>>    /** @} */
>>>    #endif
>>
>> _______________________________________________
>> vlc-devel mailing list
>> To unsubscribe or modify your subscription options:
>> https://mailman.videolan.org/listinfo/vlc-devel
> 
> 
> -- 
> Rémi Denis-Courmont
> Tapiola new town, Uusimaan Republic
> 
> 
> 
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
>