Re: [PATCH 1/2] compat/win32/pthread: add pthread_once()

From: Christian Couder <christian.couder@gmail.com>
Date: Jun 26, 2020, 05:45 UTC
Message-ID: <CAP8UFD3NwKtWYL_H5fWjT-HQVd3W8ZJ49p+faDyMQ+==ttCWjA@mail.gmail.com>
In-Reply-To: <e5a10d3f2152859b75bd815c37511975057d0ab0.1593115455.git.matheus.bernardino@usp.br>

On Fri, Jun 26, 2020 at 3:00 AM Matheus Tavares <matheus.bernardino@usp.br> wrote:

>
> Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
> ---

The commit message might want to explain a bit the purpose of adding these features.

Show 5 quoted lines

> Note: the pthread_once() function is adapted from:
> https://git.libav.org/?p=libav.git;a=commitdiff;h=b22693b06d1e5d73454a65c203b4d31c1ca5b69a
>
> Which is LGPLv2.1. Should I add any notice/acknowledgment somewhere,
> besides the comment I added above the function?

Yeah, I think you should also tell in the commit message where the code comes from (along with the hash of the commit) and that libav is LGPLv2.1 which is compatible with GPLv2 as explained in section 3 of the LGPLv2.1.

Show 22 quoted lines

>  compat/win32/pthread.c | 22 ++++++++++++++++++++++
>  compat/win32/pthread.h |  5 +++++
>  thread-utils.c         | 11 +++++++++++
>  thread-utils.h         |  6 ++++++
>  4 files changed, 44 insertions(+)
>
> diff --git a/compat/win32/pthread.c b/compat/win32/pthread.c
> index 2e7eead42c..5a7ecbd999 100644
> --- a/compat/win32/pthread.c
> +++ b/compat/win32/pthread.c
> @@ -56,3 +56,25 @@ pthread_t pthread_self(void)
>         t.tid = GetCurrentThreadId();
>         return t;
>  }
> +
> +/* Adapted from libav's compat/w32pthreads.h. */
> +int pthread_once(pthread_once_t *once_control, void (*init_routine)(void))
> +{
> +       BOOL pending = FALSE;
> +       int ret = 0;
> +
> +       if(!InitOnceBeginInitialize(once_control, 0, &pending, NULL)) {

We put a space between "if" and the following "(". It might also be interesting to know perhaps in the commit message how much you adapted the code.

For example perhaps a good strategy would be in the commit that imports the code to do the minimal amount of change so that it builds and passes the test, and then to have another commit that adapts the style of the code.

Show 8 quoted lines

> +               ret = err_win_to_posix(GetLastError());
> +               goto out;
> +       }
> +
> +       if (pending)
> +               init_routine();
> +
> +       if(!InitOnceComplete(once_control, 0, NULL))

Space missing between "if" and the following "(".

Show 6 quoted lines

> +               ret = err_win_to_posix(GetLastError());
> +
> +out:
> +       /* POSIX doesn't allow pthread_once() to return EINTR */
> +       return ret == EINTR ? EIO : ret;
> +}