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

[Alexandre Janniaux]

Hi,

On Thu, Aug 27, 2020 at 11:37:14AM +0200, Steve Lhomme wrote:
> 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.

I'm also in favour of Romain's code style when it makes sense
though the void (void*) case is not worth the consistency.

However, I've always been told that code style didn't matter
as long as it were at least consistent within the file/function.

IMHO it's a debate with no ends. Either there is no «official
rules» or we just put a clang-format config, check style in CI
and be done with it in 5 minutes.

Regards,
--
Alexandre Janniaux
Videolabs