git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jeff King <peff@peff.net>
To: "brian m. carlson" <sandals@crustytoothpaste.net>
Cc: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>,
	Eric Sunshine <sunshine@sunshineco.com>
Subject: Re: [PATCH v2 1/1] git-compat-util: add a test balloon for C99 support
Date: Tue, 16 Nov 2021 07:19:43 -0500	[thread overview]
Message-ID: <YZOh370ZMMqSADUE@coredump.intra.peff.net> (raw)
In-Reply-To: <20211116021241.1565740-2-sandals@crustytoothpaste.net>

On Tue, Nov 16, 2021 at 02:12:41AM +0000, 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.

I like the idea of being able to assume C99. And I know this list is
just "here are some things we could do". But I'd like to express caution
over variable length arrays. We've already had problems with alloca()
causing stack exhaustion, and VLAs are basically the same thing. And the
worst part is there's no way to recover; you just get a segfault.

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

This part makes sense to me. The macro check will complain if any
compiler isn't C99. But this hunk seems like it is going to cause
headaches:

> diff --git a/Makefile b/Makefile
> index 12be39ac49..893d533d22 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.

Do most compilers understand -std=gnu99? It seems like we're breaking
the out-of-the-box build for everything that isn't gcc or clang.

I understand that older versions of gcc (prior to 5.1.0, from my
digging) default to gnu89, and so they would be broken _without_ this.
So it is a tradeoff one way or the other. But somehow this seems
backwards to me. We should assume that modern compilers support C99 out
of the box, and put the burden on older ones to trigger C99 support in
whatever non-portable way they need.

I also checked clang, and it looks like it has defaulted to gnu11 since
clang-7 (I'm not sure about clang-6; its documentation wasn't clear).

-Peff

  reply	other threads:[~2021-11-16 12:21 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-11-14 21:24 [PATCH 0/1] Add a test balloon for C99 brian m. carlson
2021-11-14 21:24 ` [PATCH 1/1] git-compat-util: add a test balloon for C99 support brian m. carlson
2021-11-15  1:14   ` Ævar Arnfjörð Bjarmason
2021-11-15  1:54     ` brian m. carlson
2021-11-15  3:16   ` Eric Sunshine
2021-11-16  1:53     ` brian m. carlson
2021-11-22 11:47   ` Johannes Schindelin
2021-11-14 21:43 ` [PATCH 0/1] Add a test balloon for C99 brian m. carlson
2021-11-15  7:00 ` Junio C Hamano
2021-11-15 22:41   ` brian m. carlson
2021-11-16 19:02     ` Junio C Hamano
2021-11-17  1:51       ` brian m. carlson
2021-11-16  2:12 ` [PATCH v2 0/1] Add a test balloon for C99 support brian m. carlson
2021-11-16  2:12   ` [PATCH v2 1/1] git-compat-util: add " brian m. carlson
2021-11-16 12:19     ` Jeff King [this message]
2021-11-16 12:54       ` Ævar Arnfjörð Bjarmason
2021-11-16 14:54         ` Jeff King
2021-11-17  2:53           ` brian m. carlson
2021-11-17  3:01             ` Jeff King
2021-11-17 23:18               ` brian m. carlson
2021-11-17 23:45                 ` Carlo Arenas
2021-11-18  2:26                   ` Ævar Arnfjörð Bjarmason
2021-11-18 19:10                 ` Junio C Hamano
2021-11-17  8:49           ` Junio C Hamano
2021-11-16 19:44       ` Phillip Wood
2021-11-17  1:44       ` brian m. carlson
2021-11-17  2:58         ` Jeff King
2021-11-30 20:43 ` Microsoft's C language policy (was: [PATCH 0/1] Add a test balloon for C99) Ævar Arnfjörð Bjarmason
2021-11-30 22:37   ` brian m. carlson
2021-12-01  1:40 ` [PATCH v3 0/1] Add a test balloon for C99 support brian m. carlson
2021-12-01  1:40   ` [PATCH v3 1/1] git-compat-util: add " brian m. carlson
2021-12-02 17:38     ` 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=YZOh370ZMMqSADUE@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=sandals@crustytoothpaste.net \
    --cc=sunshine@sunshineco.com \
    /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).