git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Christian Couder <christian.couder@gmail.com>
To: Matheus Tavares <matheus.bernardino@usp.br>
Cc: git <git@vger.kernel.org>,
	"brian m. carlson" <sandals@crustytoothpaste.net>,
	Johannes Sixt <j6t@kdbg.org>,
	Jonathan Tan <jonathantanmy@google.com>,
	Jeff King <peff@peff.net>,
	Johannes Schindelin <Johannes.Schindelin@gmx.de>
Subject: Re: [PATCH 1/2] compat/win32/pthread: add pthread_once()
Date: Fri, 26 Jun 2020 07:45:03 +0200	[thread overview]
Message-ID: <CAP8UFD3NwKtWYL_H5fWjT-HQVd3W8ZJ49p+faDyMQ+==ttCWjA@mail.gmail.com> (raw)
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.

> 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.

>  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.

> +               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 "(".

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

  reply	other threads:[~2020-06-26  5:45 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-24 22:29 [RFC] Thread safety in some low-level functions Matheus Tavares Bernardino
2020-06-24 22:52 ` Matheus Tavares Bernardino
2020-06-25  1:38   ` brian m. carlson
2020-06-25 20:32     ` [PATCH 0/2] Make oid_to_hex() thread-safe Matheus Tavares
2020-06-25 20:32       ` [PATCH 1/2] compat/win32/pthread: add pthread_once() Matheus Tavares
2020-06-26  5:45         ` Christian Couder [this message]
2020-06-26 14:13           ` Matheus Tavares Bernardino
2020-06-25 20:32       ` [PATCH 2/2] hex: make hash_to_hex_algop() and friends thread-safe Matheus Tavares
2020-06-25 23:07         ` brian m. carlson
2020-06-25 23:54           ` Matheus Tavares Bernardino
2020-06-26  0:00             ` Matheus Tavares Bernardino
2020-06-26  6:02         ` Christian Couder
2020-06-26  8:22       ` [PATCH 0/2] Make oid_to_hex() thread-safe Christian Couder
2020-06-26 16:22         ` Matheus Tavares Bernardino
2020-06-26 21:54       ` [PATCH v2 " Matheus Tavares
2020-06-26 21:54         ` [PATCH v2 1/2] compat/win32/pthread: add pthread_once() Matheus Tavares
2020-06-26 21:54         ` [PATCH v2 2/2] hex: make hash_to_hex_algop() and friends thread-safe Matheus Tavares
2020-06-29 15:11           ` Johannes Schindelin
2020-06-30 20:37             ` Matheus Tavares Bernardino
2020-07-01 11:32               ` Johannes Schindelin
2020-07-16 11:29           ` Johannes Schindelin
2020-07-18  3:09             ` Matheus Tavares Bernardino
2020-08-10 14:15               ` Johannes Schindelin
2020-07-18  3:52             ` Matheus Tavares
2020-07-26 17:41               ` René Scharfe

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAP8UFD3NwKtWYL_H5fWjT-HQVd3W8ZJ49p+faDyMQ+==ttCWjA@mail.gmail.com' \
    --to=christian.couder@gmail.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=git@vger.kernel.org \
    --cc=j6t@kdbg.org \
    --cc=jonathantanmy@google.com \
    --cc=matheus.bernardino@usp.br \
    --cc=peff@peff.net \
    --cc=sandals@crustytoothpaste.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).