Re: [PATCH 1/1] git-compat-util: add a test balloon for C99 support

4 messages, 3 authors, 2021-11-22 · open the first message on its own page

Re: [PATCH 1/1] git-compat-util: add a test balloon for C99 support

From: Johannes Schindelin <hidden>
Date: 2021-11-16 10:30:35

Hi brian,

On Sun, 14 Nov 2021, brian m. carlson wrote:
The C99 standard was released in January 1999, now 22 years ago.  It
provides a variety of useful features, including variadic arguments for
macros, declarations after statements, variable length arrays, and a
wide variety of other useful features, many of which we already use.

We'd like to take advantage of these features, but we want to be
cautious.  As far as we know, all major compilers now support C99 or a
later C standard, such as C11 or C17.  POSIX has required C99 support as
a requirement for the 2001 revision, so we can safely assume any POSIX
system which we are interested in supporting has C99.

Even MSVC, long a holdout against modern C, now supports both C11 and
C17 with an appropriate update.  Moreover, even if people are using an
older version of MSVC on these systems, they will generally need some
implementation of the standard Unix utilities for the testsuite, and GNU
coreutils, the most common option, has required C99 since 2009.
Therefore, we can safely assume that a suitable version of GCC or clang
is available to users even if their version of MSVC is not sufficiently
capable.

Let's add a test balloon to git-compat-util.h to see if anyone is using
an older compiler.  We'll add a comment telling people how to enable
this functionality on GCC and Clang, even though modern versions of both
will automatically do the right thing, and ask people still experiencing
a problem to report that to us on the list.

Note that C89 compilers don't provide the __STDC_VERSION__ macro, so we
use a well-known hack of using "- 0".  On compilers with this macro, it
doesn't change the value, and on C89 compilers, the macro will be
replaced with nothing, and our value will be 0.

Sparse is also updated with a reference to the gnu99 standard, without
which it defaults to C89.

Update the cmake configuration to require C11 for MSVC.  We do this
because this will make MSVC to use C11, since it does not explicitly
support C99.  We do this with a compiler options because setting the
C_STANDARD option does not work in our CI on MSVC and at the moment, we
don't want to require C11 for Unix compilers.
I am all in favor of this patch!

Thank you,
Dscho
quoted hunk
Signed-off-by: brian m. carlson <redacted>
---
 Makefile                            |  4 ++--
 contrib/buildsystems/CMakeLists.txt |  3 +--
 git-compat-util.h                   | 12 ++++++++++++
 3 files changed, 15 insertions(+), 4 deletions(-)
diff --git a/Makefile b/Makefile
index 12be39ac49..22d9e67542 100644
--- a/Makefile
+++ b/Makefile
@@ -1204,7 +1204,7 @@ endif
 # Set CFLAGS, LDFLAGS and other *FLAGS variables. These might be
 # tweaked by config.* below as well as the command-line, both of
 # which'll override these defaults.
-CFLAGS = -g -O2 -Wall
+CFLAGS = -g -O2 -Wall -std=gnu99
 LDFLAGS =
 CC_LD_DYNPATH = -Wl,-rpath,
 BASIC_CFLAGS = -I.
@@ -1215,7 +1215,7 @@ ARFLAGS = rcs
 PTHREAD_CFLAGS =

 # For the 'sparse' target
-SPARSE_FLAGS ?=
+SPARSE_FLAGS ?= -std=gnu99
 SP_EXTRA_FLAGS = -Wno-universal-initializer

 # For informing GIT-BUILD-OPTIONS of the SANITIZE=leak target
diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
index fd1399c440..91e8525fa9 100644
--- a/contrib/buildsystems/CMakeLists.txt
+++ b/contrib/buildsystems/CMakeLists.txt
@@ -208,7 +208,7 @@ endif()
 if(CMAKE_C_COMPILER_ID STREQUAL "MSVC")
 	set(CMAKE_RUNTIME_OUTPUT_DIRECTORY_DEBUG ${CMAKE_BINARY_DIR})
 	set(CMAKE_RUNTIME_OUTPUT_DIRECTORY_RELEASE ${CMAKE_BINARY_DIR})
-	add_compile_options(/MP)
+	add_compile_options(/MP /std:c11)
 endif()

 #default behaviour
@@ -600,7 +600,6 @@ endif()
 list(REMOVE_DUPLICATES excluded_progs)
 list(REMOVE_DUPLICATES PROGRAMS_BUILT)

-
 foreach(p ${excluded_progs})
 	list(APPEND EXCLUSION_PROGS --exclude-program ${p} )
 endforeach()
diff --git a/git-compat-util.h b/git-compat-util.h
index d70ce14286..6d995bdc0f 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -1,6 +1,18 @@
 #ifndef GIT_COMPAT_UTIL_H
 #define GIT_COMPAT_UTIL_H

+#if __STDC_VERSION__ - 0 < 199901L
+/*
+ * Git is in a testing period for mandatory C99 support in the compiler.  If
+ * your compiler is reasonably recent, you can try to enable C99 support (or,
+ * for MSVC, C11 support).  If you encounter a problem and can't enable C99
+ * support with your compiler and don't have access to one with this support,
+ * such as GCC or Clang, you can remove this #if directive, but please report
+ * the details of your system to git@vger.kernel.org.
+ */
+#error "Required C99 support is in a test phase.  Please see git-compat-util.h for more details."
+#endif
+
 #ifdef USE_MSVC_CRTDBG
 /*
  * For these to work they must appear very early in each

Re: [PATCH 1/1] git-compat-util: add a test balloon for C99 support

From: Junio C Hamano <hidden>
Date: 2021-11-17 08:29:15

Johannes Schindelin [off-list ref] writes:
quoted
Even MSVC, long a holdout against modern C, now supports both C11 and
C17 with an appropriate update.  Moreover, even if people are using an
older version of MSVC on these systems, they will generally need some
implementation of the standard Unix utilities for the testsuite, and GNU
coreutils, the most common option, has required C99 since 2009.
Therefore, we can safely assume that a suitable version of GCC or clang
is available to users even if their version of MSVC is not sufficiently
capable.
I am all in favor of this patch!
I like the direction, but ...
quoted
diff --git a/Makefile b/Makefile
index 12be39ac49..22d9e67542 100644
--- a/Makefile
+++ b/Makefile
@@ -1204,7 +1204,7 @@ endif
 # Set CFLAGS, LDFLAGS and other *FLAGS variables. These might be
 # tweaked by config.* below as well as the command-line, both of
 # which'll override these defaults.
-CFLAGS = -g -O2 -Wall
+CFLAGS = -g -O2 -Wall -std=gnu99
... as has been already pointed out, this part probably should not
be there.  It is not our intention to require gcc/clang, or to
constrain newer systems to gnu99.

Re: [PATCH 1/1] git-compat-util: add a test balloon for C99 support

From: Johannes Schindelin <hidden>
Date: 2021-11-22 11:45:01

Hi Junio & brian,

On Wed, 17 Nov 2021, Junio C Hamano wrote:
Johannes Schindelin [off-list ref] writes:
quoted
quoted
Even MSVC, long a holdout against modern C, now supports both C11 and
C17 with an appropriate update.  Moreover, even if people are using an
older version of MSVC on these systems, they will generally need some
implementation of the standard Unix utilities for the testsuite, and GNU
coreutils, the most common option, has required C99 since 2009.
Therefore, we can safely assume that a suitable version of GCC or clang
is available to users even if their version of MSVC is not sufficiently
capable.
I am all in favor of this patch!
I like the direction, but ...
quoted
quoted
diff --git a/Makefile b/Makefile
index 12be39ac49..22d9e67542 100644
--- a/Makefile
+++ b/Makefile
@@ -1204,7 +1204,7 @@ endif
 # Set CFLAGS, LDFLAGS and other *FLAGS variables. These might be
 # tweaked by config.* below as well as the command-line, both of
 # which'll override these defaults.
-CFLAGS = -g -O2 -Wall
+CFLAGS = -g -O2 -Wall -std=gnu99
... as has been already pointed out, this part probably should not
be there.  It is not our intention to require gcc/clang, or to
constrain newer systems to gnu99.
Another data point in favor of dropping this: our FreeBSD CI build reports
a compile error with this:

	[...]
	archive.c:337:35: error: '_Generic' is a C11 extension
	[-Werror,-Wc11-extensions]
			strbuf_addstr(&path_in_archive, basename(path));
							^
	/usr/include/libgen.h:61:21: note: expanded from macro 'basename'
	#define basename(x)     __generic(x, const char *, __old_basename, basename)(x)
				^
	/usr/include/sys/cdefs.h:329:2: note: expanded from macro '__generic'
		_Generic(expr, t: yes, default: no)
		^
	1 error generated.

I verified in https://github.com/gitgitgadget/git/pull/1082 that this
patch is indeed the cause of this compile error.

Ciao,
Dscho

Re: [PATCH 1/1] git-compat-util: add a test balloon for C99 support

From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2021-11-22 13:11:30

On Mon, Nov 22 2021, Johannes Schindelin wrote:
Hi Junio & brian,

On Wed, 17 Nov 2021, Junio C Hamano wrote:
quoted
Johannes Schindelin [off-list ref] writes:
quoted
quoted
Even MSVC, long a holdout against modern C, now supports both C11 and
C17 with an appropriate update.  Moreover, even if people are using an
older version of MSVC on these systems, they will generally need some
implementation of the standard Unix utilities for the testsuite, and GNU
coreutils, the most common option, has required C99 since 2009.
Therefore, we can safely assume that a suitable version of GCC or clang
is available to users even if their version of MSVC is not sufficiently
capable.
I am all in favor of this patch!
I like the direction, but ...
quoted
quoted
diff --git a/Makefile b/Makefile
index 12be39ac49..22d9e67542 100644
--- a/Makefile
+++ b/Makefile
@@ -1204,7 +1204,7 @@ endif
 # Set CFLAGS, LDFLAGS and other *FLAGS variables. These might be
 # tweaked by config.* below as well as the command-line, both of
 # which'll override these defaults.
-CFLAGS = -g -O2 -Wall
+CFLAGS = -g -O2 -Wall -std=gnu99
... as has been already pointed out, this part probably should not
be there.  It is not our intention to require gcc/clang, or to
constrain newer systems to gnu99.
Another data point in favor of dropping this: our FreeBSD CI build reports
a compile error with this:

	[...]
	archive.c:337:35: error: '_Generic' is a C11 extension
	[-Werror,-Wc11-extensions]
			strbuf_addstr(&path_in_archive, basename(path));
							^
	/usr/include/libgen.h:61:21: note: expanded from macro 'basename'
	#define basename(x)     __generic(x, const char *, __old_basename, basename)(x)
				^
	/usr/include/sys/cdefs.h:329:2: note: expanded from macro '__generic'
		_Generic(expr, t: yes, default: no)
		^
	1 error generated.

I verified in https://github.com/gitgitgadget/git/pull/1082 that this
patch is indeed the cause of this compile error.
As noted in another reply I don't think this -std=* thing is worth it,
but this isn't so much a case of breakage with this patch in particular,
but revealing an existing issue of us implicitly requiring C11 on
FreeBSD.

Whether that's worth pursuing is another matter, but it's not some
inherent issue in this approach, but just a platform-specific nit we
could fix. Either by saying -std=c11 on that platform, or presumably
defining NO_LIBGEN_H if we wanted to proceed in lockstep with
C99-but-not-C11 everyhere.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help