git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Johannes Schindelin <johannes.schindelin@gmx.de>
Cc: git@vger.kernel.org, Johannes Sixt <j6t@kdbg.org>
Subject: Re: [PATCH] Windows: only add a no-op pthread_sigmask() when needed
Date: Tue, 10 May 2016 10:57:08 -0700	[thread overview]
Message-ID: <xmqqvb2lpzij.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <26c2fb5560246fc7f980da24a239edc333864527.1462885167.git.johannes.schindelin@gmx.de> (Johannes Schindelin's message of "Tue, 10 May 2016 15:00:35 +0200 (CEST)")

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> In f924b52 (Windows: add pthread_sigmask() that does nothing,
> 2016-05-01), we introduced a no-op for Windows. However, this breaks
> building Git in Git for Windows' SDK because pthread_sigmask() is
> already a no-op there, #define'd in the pthread_signal.h header in
> /mingw64/x86_64-w64-mingw32/include/.
>
> Let's guard the definition of pthread_sigmask() in #ifndef...#endif to
> make the code compile both with modern MinGW-w64 as well as with the
> previously common MinGW headers.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>
> 	This patch is obviously based on 'next' (because 'master' does not
> 	have the referenced commit yet).

One thing that makes me wonder is what would happen when
/mingw64/x86_64-w64-mingw32/include/pthread_signal.h changes its
mind and uses "static inline" instead of "#define".  How much
control does Git-for-Windows folks have over that header file?

Also, could you explain "However" part a bit?  Obviously in _some_
environment other than "Git for Windows' SDK", the previous patch
helped you compile.  And you need to #ifdef it out, because "Git for
Windows' SDK" already has its own support.  What is that other
environment that lacks the support (hence we need our own "static
inline") and is there a way to tell "Git for Windows' SDK" from that
other environment?

What I am trying to get at is:

 (1) If the answer is "we have total control", then I am perfectly
     fine with using "#ifdef pthread_sigmask" here.

 (2) If not, i.e. "they can change the implementation to 'static
     inline' themselves", then I do not think it is prudent to use
     "#ifndef pthread_sigmask" as the conditional here--using a
     symbol that lets you check for that "other" environment and
     doing "#ifdef THAT_OTHER_ONE_THAT_LACKS_SIGMASK" would be
     safer.

Also is https://lists.gnu.org/archive/html/bug-gnulib/2015-04/msg00068.html
relevant?  Does /mingw64/x86_64-w64-mingw32/include/ implement "macro only
without function"?

> Published-As: https://github.com/dscho/git/releases/tag/mingw-sigmask-v1
>  compat/win32/pthread.h | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/compat/win32/pthread.h b/compat/win32/pthread.h
> index d336451..8df702c 100644
> --- a/compat/win32/pthread.h
> +++ b/compat/win32/pthread.h
> @@ -104,9 +104,11 @@ static inline void *pthread_getspecific(pthread_key_t key)
>  	return TlsGetValue(key);
>  }
>  
> +#ifndef pthread_sigmask
>  static inline int pthread_sigmask(int how, const sigset_t *set, sigset_t *oset)
>  {
>  	return 0;
>  }
> +#endif
>  
>  #endif /* PTHREAD_H */

  reply	other threads:[~2016-05-10 17:57 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-05-10 13:00 [PATCH] Windows: only add a no-op pthread_sigmask() when needed Johannes Schindelin
2016-05-10 17:57 ` Junio C Hamano [this message]
2016-05-11 15:06   ` Johannes Schindelin
2016-05-11 21:01     ` Junio C Hamano
2016-05-11 15:10 ` [PATCH v2] " Johannes Schindelin

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

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

  git send-email \
    --in-reply-to=xmqqvb2lpzij.fsf@gitster.mtv.corp.google.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=j6t@kdbg.org \
    --cc=johannes.schindelin@gmx.de \
    /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.
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 NNTP newsgroup(s).