[vlc-devel] [RFC 1/2] executor: introduce new executor API

[Steve Lhomme]

On 2020-08-27 10:23, Romain Vimont wrote:
> On Thu, Aug 27, 2020 at 07:09:00AM +0200, Steve Lhomme wrote:
>> On 2020-08-26 19:19, Romain Vimont wrote:
>>> Introduce a new API to execute "runnables". The execution can be
>>> automatically interrupted on timeout or via an explicit cancelation
>>> request.
>>> ---
>>>    include/vlc_executor.h | 194 +++++++++++++++
>>>    src/Makefile.am        |   1 +
>>>    src/libvlccore.sym     |   4 +
>>>    src/misc/executor.c    | 545 +++++++++++++++++++++++++++++++++++++++++
>>>    4 files changed, 744 insertions(+)
>>>    create mode 100644 include/vlc_executor.h
>>>    create mode 100644 src/misc/executor.c
>>>
>>> diff --git a/include/vlc_executor.h b/include/vlc_executor.h
>>> new file mode 100644
>>> index 0000000000..4dd8b93628
>>> --- /dev/null
>>> +++ b/include/vlc_executor.h
>>> @@ -0,0 +1,194 @@
>>> +/*****************************************************************************
>>> + * vlc_executor.h
>>> + *****************************************************************************
>>> + * Copyright (C) 2020 Videolabs, VLC authors and VideoLAN
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify it
>>> + * under the terms of the GNU Lesser General Public License as published by
>>> + * the Free Software Foundation; either version 2.1 of the License, or
>>> + * (at your option) any later version.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>>> + * GNU Lesser General Public License for more details.
>>> + *
>>> + * You should have received a copy of the GNU Lesser General Public License
>>> + * along with this program; if not, write to the Free Software Foundation,
>>> + * Inc., 51 Franklin Street, Fifth Floor, Boston MA 02110-1301, USA.
>>> + *****************************************************************************/
>>> +
>>> +#ifndef VLC_EXECUTOR_H
>>> +#define VLC_EXECUTOR_H
>>> +
>>> +#include <vlc_common.h>
>>> +#include <vlc_tick.h>
>>> +
>>> +# ifdef __cplusplus
>>> +extern "C" {
>>> +# endif
>>> +
>>> +/** Executor type (opaque) */
>>> +typedef struct vlc_executor vlc_executor_t;
>>> +
>>> +/**
>>> + * A Runnable is intended to be run from another thread by an executor.
>>> + */
>>> +struct vlc_runnable {
>>> +
>>> +    /**
>>> +     * This function is to be executed by a vlc_executor_t.
>>> +     *
>>> +     * It must implement the actions (arbitrarily long) to execute from an
>>> +     * executor thread, synchronously. As soon as run() returns, the execution
>>> +     * of this runnable is complete.
>>> +     *
>>> +     * After the runnable is submitted to an executor via
>>> +     * vlc_executor_Submit(), the run() function is executed at most once (zero
>>> +     * if the execution is canceled before it was started).
>>> +     *
>>> +     * It may not be NULL.
>>> +     *
>>> +     * \param userdata the userdata provided to vlc_executor_Submit()
>>> +     */
>>> +    void
>>
>> Why
>>
>>> +    (*run)(void *userdata);
>>
>> Do you
>> use two lines
>> for this ?
>>
>> When grepping for code I always stumble such unreadable lines. I have to
>> open the files for each of them to read the actual prototype.
> 
> We adopted this "code style" with Thomas on the playlist/player.
> 
> The reason is that the content before the function name may be very
> long, and we always align the arguments when there is not enough space:
> 
> static inline const struct vlc_something_t *vlc_something_GetSomething(struct vlc_something_t *something,
>                                                                         vlc_tick_t tick,
>                                                                         const char *yet_another_arg);
> 
> I prefer:
> 
> static inline const struct vlc_something_t *
> vlc_something_GetSomething(struct vlc_something_t *something,
>                             vlc_tick_t tick,
>                             const char *yet_another_arg);
> 
> (both if the column width limit is 80 or 100 chars)
> 
> For consistency, I do the same even for a simple "void".

A rule (that you decided on your own) that makes rare cases OK and 
normal cases horrible is not a good rule.