[PATCH] strbuf: use designated initializers in STRBUF_INIT

[PATCH] strbuf: use designated initializers in STRBUF_INIT

[Jeff King]

On Sun, Jul 09, 2017 at 10:05:49AM -0700, Junio C Hamano wrote:

> René Scharfe <l.s.r@web.de> writes:
> 
> > I wonder when we can begin to target C99 in git's source, though. :)
> 
> Let's get the ball rolling by starting to use some of the useful
> features like designated initializers, perhaps, in a small, critical
> and reasonably stable part of the system that anybody must compile,
> leave it in one full release cycle or two, and when we hear nobody
> complains, introduce it en masse for the remainder of the system?
> 
> That way, we will see if there are people who need pre-C99 soon
> enough, and we won't have to scramble reverting too many changes
> when it happens.

Neat idea. Something like this?

-- >8 --
Subject: [PATCH] strbuf: use designated initializers in STRBUF_INIT

There are certain C99 features that might be nice to use in
our code base, but we've hesitated to do so in order to
avoid breaking compatibility with older compilers. But we
don't actually know if people are even using pre-C99
compilers these days.

One way to figure that out is to introduce a very small use
of a feature, and see if anybody complains. The strbuf code
is a good place to do this for a few reasons:

  - it always gets compiled, no matter which Makefile knobs
    have been tweaked.

  - it's very stable; this definition hasn't changed in a
    long time and is not likely to (so if we have to revert,
    it's unlikely to cause headaches)

If this patch can survive a few releases without complaint,
then we can feel more confident that designated initializers
are widely supported by our user base.  It also is an
indication that other C99 features may be supported, but not
a guarantee (e.g., gcc had designated initializers before
C99 existed).

And if we do get complaints, then we'll have gained some
data and we can easily revert this patch.

Signed-off-by: Jeff King <peff@peff.net>
---
I suspected we could also do something with __STDC_VERSION__, though I
wonder what compilers set it to when not in standards-compliant mode.
gcc-6 claims C11 when no specific -std flag is given.

And obviously before releasing this or anything similar, it would be
nice to see results from people building pu. I'm especially curious
whether MSVC would work with this (or if people even still use it, since
Git for Windows is pretty mature?).

 strbuf.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/strbuf.h b/strbuf.h
index 2075384e0..e705b94db 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -68,7 +68,7 @@ struct strbuf {
 };
 
 extern char strbuf_slopbuf[];
-#define STRBUF_INIT  { 0, 0, strbuf_slopbuf }
+#define STRBUF_INIT  { .alloc = 0, .len = 0, .buf = strbuf_slopbuf }
 
 /**
  * Life Cycle Functions
-- 
2.13.2.1071.gcd8104b61

Re: [PATCH] strbuf: use designated initializers in STRBUF_INIT

[Ben Peart]

On 7/10/2017 3:03 AM, Jeff King wrote:
> On Sun, Jul 09, 2017 at 10:05:49AM -0700, Junio C Hamano wrote:
> 
>> René Scharfe <l.s.r@web.de> writes:
>>
>>> I wonder when we can begin to target C99 in git's source, though. :)
>>
>> Let's get the ball rolling by starting to use some of the useful
>> features like designated initializers, perhaps, in a small, critical
>> and reasonably stable part of the system that anybody must compile,
>> leave it in one full release cycle or two, and when we hear nobody
>> complains, introduce it en masse for the remainder of the system?
>>
>> That way, we will see if there are people who need pre-C99 soon
>> enough, and we won't have to scramble reverting too many changes
>> when it happens.
> 
> Neat idea. Something like this?
> 
> -- >8 --
> Subject: [PATCH] strbuf: use designated initializers in STRBUF_INIT
> 
> There are certain C99 features that might be nice to use in
> our code base, but we've hesitated to do so in order to
> avoid breaking compatibility with older compilers. But we
> don't actually know if people are even using pre-C99
> compilers these days.
> 
> One way to figure that out is to introduce a very small use
> of a feature, and see if anybody complains. The strbuf code
> is a good place to do this for a few reasons:
> 
>    - it always gets compiled, no matter which Makefile knobs
>      have been tweaked.
> 
>    - it's very stable; this definition hasn't changed in a
>      long time and is not likely to (so if we have to revert,
>      it's unlikely to cause headaches)
> 
> If this patch can survive a few releases without complaint,
> then we can feel more confident that designated initializers
> are widely supported by our user base.  It also is an
> indication that other C99 features may be supported, but not
> a guarantee (e.g., gcc had designated initializers before
> C99 existed).
> 

Correct.  MSVC also supports designated initializers but does not fully 
support C99.

> And if we do get complaints, then we'll have gained some
> data and we can easily revert this patch.
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I suspected we could also do something with __STDC_VERSION__, though I
> wonder what compilers set it to when not in standards-compliant mode.
> gcc-6 claims C11 when no specific -std flag is given.
> 
> And obviously before releasing this or anything similar, it would be
> nice to see results from people building pu. I'm especially curious
> whether MSVC would work with this (or if people even still use it, since
> Git for Windows is pretty mature?).
> 

We do use MSVC internally as that gives us access to the great debuggers 
and profilers on the Windows platform.  Fortunately, this particular C99 
construct _is_ supported by MSVC.  I applied the patch below and 
complied it with both MSVC and gcc for Windows and both builds succeeded.

>   strbuf.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/strbuf.h b/strbuf.h
> index 2075384e0..e705b94db 100644
> --- a/strbuf.h
> +++ b/strbuf.h
> @@ -68,7 +68,7 @@ struct strbuf {
>   };
>   
>   extern char strbuf_slopbuf[];
> -#define STRBUF_INIT  { 0, 0, strbuf_slopbuf }
> +#define STRBUF_INIT  { .alloc = 0, .len = 0, .buf = strbuf_slopbuf }
>   
>   /**
>    * Life Cycle Functions
> 

Re: [PATCH] strbuf: use designated initializers in STRBUF_INIT

[Jeff King]

On Mon, Jul 10, 2017 at 10:57:57AM -0400, Ben Peart wrote:

> > If this patch can survive a few releases without complaint,
> > then we can feel more confident that designated initializers
> > are widely supported by our user base.  It also is an
> > indication that other C99 features may be supported, but not
> > a guarantee (e.g., gcc had designated initializers before
> > C99 existed).
> 
> Correct.  MSVC also supports designated initializers but does not fully
> support C99.

Out of curiosity, does MSVC define __STDC_VERSION__, and if so, to what?

> > And obviously before releasing this or anything similar, it would be
> > nice to see results from people building pu. I'm especially curious
> > whether MSVC would work with this (or if people even still use it, since
> > Git for Windows is pretty mature?).
> 
> We do use MSVC internally as that gives us access to the great debuggers and
> profilers on the Windows platform.  Fortunately, this particular C99
> construct _is_ supported by MSVC.  I applied the patch below and complied it
> with both MSVC and gcc for Windows and both builds succeeded.

Thanks. This kind of prompt testing and response is very appreciated. It
is unfortunate if we have to pick and choose C99-isms rather than using
the whole thing as a base. But that's probably just reality.

-Peff

Re: [PATCH] strbuf: use designated initializers in STRBUF_INIT

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> That way, we will see if there are people who need pre-C99 soon
>> enough, and we won't have to scramble reverting too many changes
>> when it happens.
>
> Neat idea. Something like this?

Yes, your log message said everything I wanted to say, including
possiblity that some compilers may have specific features without
supporting all of c99.

We accidentally started using "trailing comma at the end of enum
definition is allowed in c99", and we know it has been safe at least
for a cycle.  Credits goes to Brandon for 4538eef5 ("grep: add
submodules as a grep source type", 2016-12-16).

> -- >8 --
> Subject: [PATCH] strbuf: use designated initializers in STRBUF_INIT
>
> There are certain C99 features that might be nice to use in
> our code base, but we've hesitated to do so in order to
> avoid breaking compatibility with older compilers. But we
> don't actually know if people are even using pre-C99
> compilers these days.
>
> One way to figure that out is to introduce a very small use
> of a feature, and see if anybody complains. The strbuf code
> is a good place to do this for a few reasons:
>
>   - it always gets compiled, no matter which Makefile knobs
>     have been tweaked.
>
>   - it's very stable; this definition hasn't changed in a
>     long time and is not likely to (so if we have to revert,
>     it's unlikely to cause headaches)
>
> If this patch can survive a few releases without complaint,
> then we can feel more confident that designated initializers
> are widely supported by our user base.  It also is an
> indication that other C99 features may be supported, but not
> a guarantee (e.g., gcc had designated initializers before
> C99 existed).
>
> And if we do get complaints, then we'll have gained some
> data and we can easily revert this patch.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I suspected we could also do something with __STDC_VERSION__, though I
> wonder what compilers set it to when not in standards-compliant mode.
> gcc-6 claims C11 when no specific -std flag is given.
>
> And obviously before releasing this or anything similar, it would be
> nice to see results from people building pu. I'm especially curious
> whether MSVC would work with this (or if people even still use it, since
> Git for Windows is pretty mature?).
>
>  strbuf.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/strbuf.h b/strbuf.h
> index 2075384e0..e705b94db 100644
> --- a/strbuf.h
> +++ b/strbuf.h
> @@ -68,7 +68,7 @@ struct strbuf {
>  };
>  
>  extern char strbuf_slopbuf[];
> -#define STRBUF_INIT  { 0, 0, strbuf_slopbuf }
> +#define STRBUF_INIT  { .alloc = 0, .len = 0, .buf = strbuf_slopbuf }
>  
>  /**
>   * Life Cycle Functions

Re: [PATCH] strbuf: use designated initializers in STRBUF_INIT

[Andreas Schwab]

On Jul 10 2017, Jeff King <peff@peff.net> wrote:

> If this patch can survive a few releases without complaint,
> then we can feel more confident that designated initializers
> are widely supported by our user base.  It also is an
> indication that other C99 features may be supported, but not
> a guarantee (e.g., gcc had designated initializers before
> C99 existed).

Note that the GNU C designated initializers initially used a different
syntax than the one C99 adopted.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] strbuf: use designated initializers in STRBUF_INIT

[Stefan Beller]

On Mon, Jul 10, 2017 at 9:44 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>
>>> That way, we will see if there are people who need pre-C99 soon
>>> enough, and we won't have to scramble reverting too many changes
>>> when it happens.
>>
>> Neat idea. Something like this?
>
> Yes, your log message said everything I wanted to say, including
> possiblity that some compilers may have specific features without
> supporting all of c99.
>
> We accidentally started using "trailing comma at the end of enum
> definition is allowed in c99", and we know it has been safe at least
> for a cycle.  Credits goes to Brandon for 4538eef5 ("grep: add
> submodules as a grep source type", 2016-12-16).

Credit goes to Brandon for spotting it, but the introduction of
"trailing comma at the end of enum definition is allowed in c99"
is e1327023ea (grep: refactor the concept of "grep source" into
an object, 2012-02-02) IMHO, which is more time that proved this
feature being supported on all compilers.

Thanks for getting the ball rolling, just wondering if the patch needs
a comment in the code. The commit message is very thorough though.

Thanks,
Stefan

Re: [PATCH] strbuf: use designated initializers in STRBUF_INIT

[Ben Peart]

On 7/10/2017 12:04 PM, Jeff King wrote:
> On Mon, Jul 10, 2017 at 10:57:57AM -0400, Ben Peart wrote:
> 
>>> If this patch can survive a few releases without complaint,
>>> then we can feel more confident that designated initializers
>>> are widely supported by our user base.  It also is an
>>> indication that other C99 features may be supported, but not
>>> a guarantee (e.g., gcc had designated initializers before
>>> C99 existed).
>>
>> Correct.  MSVC also supports designated initializers but does not fully
>> support C99.
> 
> Out of curiosity, does MSVC define __STDC_VERSION__, and if so, to what?

It does not. While it does support a subset of C99's features, it 
doesn't support the entirety of that language so defining 
__STDC_VERSION__ would be inaccurate and misleading.

> 
>>> And obviously before releasing this or anything similar, it would be
>>> nice to see results from people building pu. I'm especially curious
>>> whether MSVC would work with this (or if people even still use it, since
>>> Git for Windows is pretty mature?).
>>
>> We do use MSVC internally as that gives us access to the great debuggers and
>> profilers on the Windows platform.  Fortunately, this particular C99
>> construct _is_ supported by MSVC.  I applied the patch below and complied it
>> with both MSVC and gcc for Windows and both builds succeeded.
> 
> Thanks. This kind of prompt testing and response is very appreciated. It
> is unfortunate if we have to pick and choose C99-isms rather than using
> the whole thing as a base. But that's probably just reality.
> 
> -Peff
> 

Re: [PATCH] strbuf: use designated initializers in STRBUF_INIT

[Johannes Sixt]

Am 10.07.2017 um 09:03 schrieb Jeff King:
> On Sun, Jul 09, 2017 at 10:05:49AM -0700, Junio C Hamano wrote:
> 
>> René Scharfe <l.s.r@web.de> writes:
>>
>>> I wonder when we can begin to target C99 in git's source, though. :)
>>
>> Let's get the ball rolling...

Good to know that you do not resist moving to a more modern build 
environment.

>> by starting to use some of the useful
>> features like designated initializers,

It's a pity, though, that you do not suggest something even more useful, 
such as C++14.

> Subject: [PATCH] strbuf: use designated initializers in STRBUF_INIT

> -#define STRBUF_INIT  { 0, 0, strbuf_slopbuf }
> +#define STRBUF_INIT  { .alloc = 0, .len = 0, .buf = strbuf_slopbuf }

While this may serve as a test balloon, changing STRBUF_INIT, or any of 
those _INIT macros, is actually the least interesting. The interesting 
instances are initializations for which we do *not* have a macro.

-- Hannes

Re: [PATCH] strbuf: use designated initializers in STRBUF_INIT

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> It's a pity, though, that you do not suggest something even more
> useful, such as C++14.

I cannot tell if this part is tongue-in-cheek (especially the "++"),
so I will ignore it to avoid wasting time.

>> Subject: [PATCH] strbuf: use designated initializers in STRBUF_INIT
>
>> -#define STRBUF_INIT  { 0, 0, strbuf_slopbuf }
>> +#define STRBUF_INIT  { .alloc = 0, .len = 0, .buf = strbuf_slopbuf }
>
> While this may serve as a test balloon, changing STRBUF_INIT, or any
> of those _INIT macros, is actually the least interesting. The
> interesting instances are initializations for which we do *not* have a
> macro.

I am not sure what negative impact you think the macro-ness would
have to the validity of the result from this test balloon.  An old
compiler that does not understand designated initializer syntax
would fail to compile both the same way, no?

	struct strbuf buf0 = STRBUF_INIT;
	struct strbuf buf1 = { .alloc = 0, .len = 0, .buf = strbuf_slopbuf };

Re: [PATCH] strbuf: use designated initializers in STRBUF_INIT

[Johannes Sixt]

Am 10.07.2017 um 22:38 schrieb Junio C Hamano:
> Johannes Sixt <j6t@kdbg.org> writes:
> 
>> It's a pity, though, that you do not suggest something even more
>> useful, such as C++14.
> 
> I cannot tell if this part is tongue-in-cheek (especially the "++"),
> so I will ignore it to avoid wasting time.

Actually, I'm serious.

>>> Subject: [PATCH] strbuf: use designated initializers in STRBUF_INIT
>>
>>> -#define STRBUF_INIT  { 0, 0, strbuf_slopbuf }
>>> +#define STRBUF_INIT  { .alloc = 0, .len = 0, .buf = strbuf_slopbuf }
>>
>> While this may serve as a test balloon, changing STRBUF_INIT, or any
>> of those _INIT macros, is actually the least interesting. The
>> interesting instances are initializations for which we do *not* have a
>> macro.
> 
> I am not sure what negative impact you think the macro-ness would
> have to the validity of the result from this test balloon.  An old
> compiler that does not understand designated initializer syntax
> would fail to compile both the same way, no?
> 
> 	struct strbuf buf0 = STRBUF_INIT;
> 	struct strbuf buf1 = { .alloc = 0, .len = 0, .buf = strbuf_slopbuf };

I said it is uninteresting, not that there is a negative impact. There 
is simply nothing gained for strbuf users: They would use STRBUF_INIT 
before and after the change and would not benefit from designated 
initializers.

This change may serve well as a test balloon, but not as an example of 
the sort of changes that we would want to see later (of the kind "change 
FOO_INIT macro to use designated initializers"; they are just code churn).

-- Hannes

Re: [PATCH] strbuf: use designated initializers in STRBUF_INIT

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

>> I am not sure what negative impact you think the macro-ness would
>> have to the validity of the result from this test balloon.  An old
>> compiler that does not understand designated initializer syntax
>> would fail to compile both the same way, no?
>>
>> 	struct strbuf buf0 = STRBUF_INIT;
>> 	struct strbuf buf1 = { .alloc = 0, .len = 0, .buf = strbuf_slopbuf };
>
> I said it is uninteresting, not that there is a negative impact. There
> is simply nothing gained for strbuf users: They would use STRBUF_INIT
> before and after the change and would not benefit from designated
> initializers.
>
> This change may serve well as a test balloon, but not as an example of
> the sort of changes that we would want to see later (of the kind
> "change FOO_INIT macro to use designated initializers"; they are just
> code churn).

Oh, absolutely.

Here is another possible test balloon, that may actually be useful
as an example.  I think there is a topic in flight that touches this
array, unfortunately, so I probably would find another one that is
more stable for a real follow-up patch to the one from Peff.

 diff.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/diff.c b/diff.c
index 00b4c86698..b3864a2e03 100644
--- a/diff.c
+++ b/diff.c
@@ -47,15 +47,15 @@ static long diff_algorithm;
 static unsigned ws_error_highlight_default = WSEH_NEW;
 
 static char diff_colors[][COLOR_MAXLEN] = {
-	GIT_COLOR_RESET,
-	GIT_COLOR_NORMAL,	/* CONTEXT */
-	GIT_COLOR_BOLD,		/* METAINFO */
-	GIT_COLOR_CYAN,		/* FRAGINFO */
-	GIT_COLOR_RED,		/* OLD */
-	GIT_COLOR_GREEN,	/* NEW */
-	GIT_COLOR_YELLOW,	/* COMMIT */
-	GIT_COLOR_BG_RED,	/* WHITESPACE */
-	GIT_COLOR_NORMAL,	/* FUNCINFO */
+	[DIFF_RESET] = GIT_COLOR_RESET,
+	[DIFF_CONTEXT] = GIT_COLOR_NORMAL,
+	[DIFF_METAINFO] = GIT_COLOR_BOLD,
+	[DIFF_FRAGINFO] = GIT_COLOR_CYAN,
+	[DIFF_FILE_OLD] = GIT_COLOR_RED,
+	[DIFF_FILE_NEW] = GIT_COLOR_GREEN,
+	[DIFF_COMMIT] = GIT_COLOR_YELLOW,
+	[DIFF_WHITESPACE] = GIT_COLOR_BG_RED,
+	[DIFF_FUNCINFO] = GIT_COLOR_NORMAL,
 };
 
 static NORETURN void die_want_option(const char *option_name)

Re: [PATCH] strbuf: use designated initializers in STRBUF_INIT

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> On Mon, Jul 10, 2017 at 9:44 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Credit goes to Brandon for spotting it, but the introduction of
> "trailing comma at the end of enum definition is allowed in c99"
> is e1327023ea (grep: refactor the concept of "grep source" into
> an object, 2012-02-02) IMHO, which is more time that proved this
> feature being supported on all compilers.

Yup, I did dig down to that commit earlier when I wrote

  https://public-inbox.org/git/xmqqlgolm2jk.fsf@gitster.mtv.corp.google.com/

I just forgot about it, as very many things going on the list ;-)

Thanks.

Re: [PATCH] strbuf: use designated initializers in STRBUF_INIT

[Brandon Williams]

On 07/10, Jeff King wrote:
> On Sun, Jul 09, 2017 at 10:05:49AM -0700, Junio C Hamano wrote:
> 
> > René Scharfe <l.s.r@web.de> writes:
> > 
> > > I wonder when we can begin to target C99 in git's source, though. :)
> > 
> > Let's get the ball rolling by starting to use some of the useful
> > features like designated initializers, perhaps, in a small, critical
> > and reasonably stable part of the system that anybody must compile,
> > leave it in one full release cycle or two, and when we hear nobody
> > complains, introduce it en masse for the remainder of the system?
> > 
> > That way, we will see if there are people who need pre-C99 soon
> > enough, and we won't have to scramble reverting too many changes
> > when it happens.
> 
> Neat idea. Something like this?
> 
> -- >8 --
> Subject: [PATCH] strbuf: use designated initializers in STRBUF_INIT
> 
> There are certain C99 features that might be nice to use in
> our code base, but we've hesitated to do so in order to
> avoid breaking compatibility with older compilers. But we
> don't actually know if people are even using pre-C99
> compilers these days.
> 
> One way to figure that out is to introduce a very small use
> of a feature, and see if anybody complains. The strbuf code
> is a good place to do this for a few reasons:
> 
>   - it always gets compiled, no matter which Makefile knobs
>     have been tweaked.
> 
>   - it's very stable; this definition hasn't changed in a
>     long time and is not likely to (so if we have to revert,
>     it's unlikely to cause headaches)
> 
> If this patch can survive a few releases without complaint,
> then we can feel more confident that designated initializers
> are widely supported by our user base.  It also is an
> indication that other C99 features may be supported, but not
> a guarantee (e.g., gcc had designated initializers before
> C99 existed).
> 
> And if we do get complaints, then we'll have gained some
> data and we can easily revert this patch.
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I suspected we could also do something with __STDC_VERSION__, though I
> wonder what compilers set it to when not in standards-compliant mode.
> gcc-6 claims C11 when no specific -std flag is given.
> 
> And obviously before releasing this or anything similar, it would be
> nice to see results from people building pu. I'm especially curious
> whether MSVC would work with this (or if people even still use it, since
> Git for Windows is pretty mature?).
> 
>  strbuf.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/strbuf.h b/strbuf.h
> index 2075384e0..e705b94db 100644
> --- a/strbuf.h
> +++ b/strbuf.h
> @@ -68,7 +68,7 @@ struct strbuf {
>  };
>  
>  extern char strbuf_slopbuf[];
> -#define STRBUF_INIT  { 0, 0, strbuf_slopbuf }
> +#define STRBUF_INIT  { .alloc = 0, .len = 0, .buf = strbuf_slopbuf }

I love that this is happening!  And maybe someday soon we can do:

  for (int i = 0; i < n; i++)

So that we can scope loop variables to the loops themselves.

-- 
Brandon Williams

Re: [PATCH] strbuf: use designated initializers in STRBUF_INIT

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 590 bytes --]

On Mon, Jul 10, 2017 at 09:57:40PM +0200, Johannes Sixt wrote:
> It's a pity, though, that you do not suggest something even more useful,
> such as C++14.

I have tried compiling Git with a C++ compiler, so that I could test
whether that was a viable alternative for MSVC in case its C++ mode
supported features its C mode did not.  Let's just say that the
compilation aborted very quickly and I gave up after a few minutes.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

Re: [PATCH] strbuf: use designated initializers in STRBUF_INIT

[Stefan Beller]

On Mon, Jul 10, 2017 at 5:05 PM, brian m. carlson
<sandals@crustytoothpaste.net> wrote:
> On Mon, Jul 10, 2017 at 09:57:40PM +0200, Johannes Sixt wrote:
>> It's a pity, though, that you do not suggest something even more useful,
>> such as C++14.
>
> I have tried compiling Git with a C++ compiler, so that I could test
> whether that was a viable alternative for MSVC in case its C++ mode
> supported features its C mode did not.  Let's just say that the
> compilation aborted very quickly and I gave up after a few minutes.

... because we use reserved C++ keywords such as 'new' as variable names?

Re: [PATCH] strbuf: use designated initializers in STRBUF_INIT

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1047 bytes --]

On Mon, Jul 10, 2017 at 05:07:43PM -0700, Stefan Beller wrote:
> On Mon, Jul 10, 2017 at 5:05 PM, brian m. carlson
> <sandals@crustytoothpaste.net> wrote:
> > On Mon, Jul 10, 2017 at 09:57:40PM +0200, Johannes Sixt wrote:
> >> It's a pity, though, that you do not suggest something even more useful,
> >> such as C++14.
> >
> > I have tried compiling Git with a C++ compiler, so that I could test
> > whether that was a viable alternative for MSVC in case its C++ mode
> > supported features its C mode did not.  Let's just say that the
> > compilation aborted very quickly and I gave up after a few minutes.
> 
> ... because we use reserved C++ keywords such as 'new' as variable names?

Yes, that was part of it ("template" stuck out to me).  I don't remember
all the issues, but they seemed quite numerous.  I'm sure it can be
done, though, if it's valuable to someone.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

Re: [PATCH] strbuf: use designated initializers in STRBUF_INIT

[Jeff King]

On Mon, Jul 10, 2017 at 11:11:35PM +0200, Johannes Sixt wrote:

> > I am not sure what negative impact you think the macro-ness would
> > have to the validity of the result from this test balloon.  An old
> > compiler that does not understand designated initializer syntax
> > would fail to compile both the same way, no?
> > 
> > 	struct strbuf buf0 = STRBUF_INIT;
> > 	struct strbuf buf1 = { .alloc = 0, .len = 0, .buf = strbuf_slopbuf };
> 
> I said it is uninteresting, not that there is a negative impact. There is
> simply nothing gained for strbuf users: They would use STRBUF_INIT before
> and after the change and would not benefit from designated initializers.
> 
> This change may serve well as a test balloon, but not as an example of the
> sort of changes that we would want to see later (of the kind "change
> FOO_INIT macro to use designated initializers"; they are just code churn).

But that is exactly the point. First the test balloon, wait a release or
two, and then make real useful changes. The purpose of the test balloon
is to gather data with minimal impact. To be useful, the changes would
be pervasive, and that would not have minimal impact.

-Peff

Re: [PATCH] strbuf: use designated initializers in STRBUF_INIT

[Mike Hommey]

On Mon, Jul 10, 2017 at 10:57:57AM -0400, Ben Peart wrote:
> Correct.  MSVC also supports designated initializers but does not fully
> support C99.

Precision: *recent versions* of MSVC support designated initializer.
2013 introduced them, but there were bugs until 2015, see e.g.
https://stackoverflow.com/questions/24090739/possible-compiler-bug-in-msvc12-vs2013-with-designated-initializer

Mike

Re: [PATCH] strbuf: use designated initializers in STRBUF_INIT

[Johannes Sixt]

Am 11.07.2017 um 02:05 schrieb brian m. carlson:
> On Mon, Jul 10, 2017 at 09:57:40PM +0200, Johannes Sixt wrote:
>> It's a pity, though, that you do not suggest something even more useful,
>> such as C++14.
> 
> I have tried compiling Git with a C++ compiler, so that I could test
> whether that was a viable alternative for MSVC in case its C++ mode
> supported features its C mode did not.  Let's just say that the
> compilation aborted very quickly and I gave up after a few minutes.

It's 3 cleanup patches and one hacky patch with this size:

  80 files changed, 899 insertions(+), 807 deletions(-)

to compile with C++. It passed the test suite last time I tried. Getting 
rid of the remaining 1000+ -fpermissive warnings is a different matter, 
though.

I can revive the patches if there is interest.

-- Hannes

Re: [PATCH] strbuf: use designated initializers in STRBUF_INIT

[Junio C Hamano]

Ben Peart <peartben@gmail.com> writes:

>> If this patch can survive a few releases without complaint,
>> then we can feel more confident that designated initializers
>> are widely supported by our user base.  It also is an
>> indication that other C99 features may be supported, but not
>> a guarantee (e.g., gcc had designated initializers before
>> C99 existed).
>
> Correct.  MSVC also supports designated initializers but does not
> fully support C99.

Thanks for a datapoint.

Just so that people do not misunderstand, it is not our goal to
declare that now you need a fully C99 compiler to build Git.

When deciding what shell scripting tools with what options in our
scripted Porcelains and tests, we use POSIX.1 as a rough guideline.
We say "let's not use this, as it is not even in POSIX" but we never
say "use of it is OK because it is in POSIX", and we sometimes even
say "even it is in POSIX, various implementation of it is buggy and
it does not work properly in practice" to certain things [*1*].

C89 has been the base of such a guideline for our C programs, and
people must not to view this patch as an attempt to raise the base
to C99.  It is rather an attempt to see how far we can safely raise
the base by checking some selected useful new features [*2*] that we
have had occasions to wish that they were available, and designated
initializer for structure fields is one of them.

We may find out that, after starting with "C89, plus this and that
feature that are known to be commonly supported", the guideline may
become more succinct to say "C99, minus this and that feature that
are not usable on some platforms", if it turns out that majority of
the systems that are not fully C99 have all of the things we care
about.  We do not know yet, and we are only at the beginning of the
journey to find it out.


[Footnote]

*1* Like `backtick` command substitutions that is very hard to make
    them nest properly and impossible to mix portably with quoting.

*2* New, relative to our current practice, that is.

Re: [PATCH] strbuf: use designated initializers in STRBUF_INIT

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1218 bytes --]

On Tue, Jul 11, 2017 at 07:24:07AM +0200, Johannes Sixt wrote:
> Am 11.07.2017 um 02:05 schrieb brian m. carlson:
> > I have tried compiling Git with a C++ compiler, so that I could test
> > whether that was a viable alternative for MSVC in case its C++ mode
> > supported features its C mode did not.  Let's just say that the
> > compilation aborted very quickly and I gave up after a few minutes.
> 
> It's 3 cleanup patches and one hacky patch with this size:
> 
>  80 files changed, 899 insertions(+), 807 deletions(-)
> 
> to compile with C++. It passed the test suite last time I tried. Getting rid
> of the remaining 1000+ -fpermissive warnings is a different matter, though.

Yeah, that's the size I was thinking.  My goal was "easily achievable in
half an hour," which it didn't seem like at the time.

> I can revive the patches if there is interest.

I'd be interested in at least a pointer to them if you have one.  I
think it might allow us to take advantage of desirable features that are
in the intersection of C99 and C++.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

Re: [PATCH] strbuf: use designated initializers in STRBUF_INIT

[Johannes Sixt]

Am 12.07.2017 um 03:26 schrieb brian m. carlson:
> On Tue, Jul 11, 2017 at 07:24:07AM +0200, Johannes Sixt wrote:
>> I can revive the patches if there is interest.
> 
> I'd be interested in at least a pointer to them if you have one.  I
> think it might allow us to take advantage of desirable features that are
> in the intersection of C99 and C++.

See here: https://github.com/j6t/git.git  c-plus-plus

I do not think that there is a lot of intersection, though.

-- Hannes

Re: [PATCH] strbuf: use designated initializers in STRBUF_INIT

[Ævar Arnfjörð Bjarmason]

On Tue, Jul 11 2017, Junio C. Hamano jotted:

> Ben Peart <peartben@gmail.com> writes:
>
>>> If this patch can survive a few releases without complaint,
>>> then we can feel more confident that designated initializers
>>> are widely supported by our user base.  It also is an
>>> indication that other C99 features may be supported, but not
>>> a guarantee (e.g., gcc had designated initializers before
>>> C99 existed).
>>
>> Correct.  MSVC also supports designated initializers but does not
>> fully support C99.
>
> Thanks for a datapoint.
>
> Just so that people do not misunderstand, it is not our goal to
> declare that now you need a fully C99 compiler to build Git.
>
> When deciding what shell scripting tools with what options in our
> scripted Porcelains and tests, we use POSIX.1 as a rough guideline.
> We say "let's not use this, as it is not even in POSIX" but we never
> say "use of it is OK because it is in POSIX", and we sometimes even
> say "even it is in POSIX, various implementation of it is buggy and
> it does not work properly in practice" to certain things [*1*].
>
> C89 has been the base of such a guideline for our C programs, and
> people must not to view this patch as an attempt to raise the base
> to C99.  It is rather an attempt to see how far we can safely raise
> the base by checking some selected useful new features [*2*] that we
> have had occasions to wish that they were available, and designated
> initializer for structure fields is one of them.
>
> We may find out that, after starting with "C89, plus this and that
> feature that are known to be commonly supported", the guideline may
> become more succinct to say "C99, minus this and that feature that
> are not usable on some platforms", if it turns out that majority of
> the systems that are not fully C99 have all of the things we care
> about.  We do not know yet, and we are only at the beginning of the
> journey to find it out.

I think in the context of this desire Johannes Sixt's "Actually, I'm
serious [about let's compile with c++]"[1] should be given some more
consideration.

I've just compiled Git with it and it passes all tests. I think the
endeavor is worthwhile in itself as C++ source-level compatibility for
git.git is clearly easy to achieve, and would effectively give us access
to more compilers (albeit in different modes, but they may discover
novel bugs that also apply to the C mode code).

Most of his patch is just avoiding C++ keywords, e.g. new -> wen, try ->
try_, this -> this_, namespace -> name_space, template -> templ
etc. It's going to be relatively easy to avoid a few keywords as
variable names, especially if we set up CI for it via Travis.

But why do it? Aside from the "more compilers" argument, we may find
that it's going to be much easier to use some C99 features we want by
having C++ source-level compatibility, and on compilers like that may
not support those features in C use the C++ mode that may support those.

I don't know enough about e.g. MSVC to say if that's the case, but if so
that might be an easier way to use some C99 features.

If not C++ support would be interesting for other reasons. Johannes
Sixt: It would be very nice to get those patches on-list.

1. 962da692-8874-191c-59d4-65b9562cf87f@kdbg.org
   (https://public-inbox.org/git/962da692-8874-191c-59d4-65b9562cf87f@kdbg.org/)

> [Footnote]
>
> *1* Like `backtick` command substitutions that is very hard to make
>     them nest properly and impossible to mix portably with quoting.
>
> *2* New, relative to our current practice, that is.

Re: [PATCH] strbuf: use designated initializers in STRBUF_INIT

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> On Tue, Jul 11 2017, Junio C. Hamano jotted:
>
>> Just so that people do not misunderstand, it is not our goal to
>> declare that now you need a fully C99 compiler to build Git.
>> ...
>
> I think in the context of this desire Johannes Sixt's "Actually, I'm
> serious [about let's compile with c++]"[1] should be given some more
> consideration.
> ...
> Most of his patch is just avoiding C++ keywords, e.g. new -> wen, try ->
> try_, this -> this_, namespace -> name_space, template -> templ
> etc. It's going to be relatively easy to avoid a few keywords as
> variable names, especially if we set up CI for it via Travis.

I am not interested at all in building the binary I personally use
with any C++ compiler, but I do not mind too much if people made it
easier for other people to do so, but only if the did it the right
way.

I do like the fact that we call two things we are comparing with a
pair of matching words, 'old' and 'new'.  When two variables or
fields have certain relationship, they should be named with words
that have constrasting meaning that explains what they are.

I would very much mind if a "let's make it buildable with C++"
effort made the code compare 'old' and 'wen'.  C++ is not that
interesting to sacrifice the readability of the code.  Don't invent
non-words like wen; don't truncate a word like 'template' in the
middle to 'templ' to make it unreadable and invite inconsistencies
(e.g. "was it templ, templa, or something else?").

If a "let's make it buildable with C++" effort needs to avoid 'new',
replace *both* 'old' and 'new' to a matching pair of words (perhaps
'pre' and 'post'?  but it is making it worse by choosing a pair with
different length.  'one' vs 'two' would be OK if there is no strong
connotation that the 'old' side is always truly older in the
function in question).

I would not mind the result of such an update that much.  We already
do use different pair of words in places that we could have used
<old, new> after all.

Having to review too many updates like that in a single sitting
would be annoying, though.

The same thing for where we use 'this'; if the existing code is
contrasting 'this' with 'that', and if your C++ effort wants to
replace 'this', you MUST replace 'that' as well so that we would
still be contrasting a pair of variables appropriately named.

Re: [PATCH] strbuf: use designated initializers in STRBUF_INIT

[Johannes Sixt]

Am 12.07.2017 um 21:12 schrieb Ævar Arnfjörð Bjarmason:
> I think in the context of this desire Johannes Sixt's "Actually, I'm
> serious [about let's compile with c++]"[1] should be given some more
> consideration.

Thank you for your support.

> I've just compiled Git with it and it passes all tests. I think the
> endeavor is worthwhile in itself as C++ source-level compatibility for
> git.git is clearly easy to achieve, and would effectively give us access
> to more compilers (albeit in different modes, but they may discover
> novel bugs that also apply to the C mode code).

Please keep in mind that my patches only show that it can be done. 
Nevertheless, the result is far, far away from valid C++ code. It can be 
compiled by GCC (thanks to its -fpermissive flag) and happens to produce 
something that works. But that does not mean that other compilers would 
grok it.

Source-level compatibility is only necessary as a stop gap in the 
transition to C++. If the transition is not going to happen, I doubt 
that there is any value. It is simply too much code churn for too little 
gain. The real gains are in the features of C++(11,14).

> But why do it? Aside from the "more compilers" argument, we may find
> that it's going to be much easier to use some C99 features we want by
> having C++ source-level compatibility, and on compilers like that may
> not support those features in C use the C++ mode that may support those.

I would be happy to learn about a C99 feature where C++ mode of some 
compiler would help.

The only C99 feature mentioned so far was designated initializers. 
Unfortunately, that actually widens the gap between C and C++, not 
lessens it. (C++ does not have designated initializers, and they are not 
on the agenda.)

> If not C++ support would be interesting for other reasons. Johannes
> Sixt: It would be very nice to get those patches on-list.

I don't think it's worth to swamp the list with the patches at this 
time. Interested parties can find them here:

https://github.com/j6t/git.git c-plus-plus

I may continue the work, slowly and as long as I find it funny. It's 
just mental exercise, unless the Git community copies the GCC Steering 
Committee's mindeset with regard to C++ in the code base 
(https://gcc.gnu.org/ml/gcc/2010-05/msg00705.html).

-- Hannes

Re: [PATCH] strbuf: use designated initializers in STRBUF_INIT

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Oh, absolutely.
>
> Here is another possible test balloon, that may actually be useful
> as an example.  I think there is a topic in flight that touches this
> array, unfortunately, so I probably would find another one that is
> more stable for a real follow-up patch to the one from Peff.

And here it is.

As to other things that we currently not allow in our codebase that
newer compilers can grok, here is what *I* think.  It is *not* meant
to be an exhaustive "what's new in C99 that is not in C89? what is
the final verdict on each of them?":

 - There were occasional cases where we wished if variable-length
   arrays, flexible array members and variadic macros were available
   in our codebase during the course of this project.  We would
   probably want to add a similar test baloon patch for each of
   them to this series that is currently two-patch long.

 - I prefer to keep decl-after-statement out of our codebase.  I
   view it as a big plus in code-readability to be able to see a
   complete list of variables that will be used in a block upfront
   before starting to read the code that uses them.

 - Corollary to the above, I do not mind to have a variable
   declaration in the initialization clause of a for() statement
   (e.g. "for (int i = 0; i < 4; i++) { ... }"), as the scoping rule
   is very sensible.  Some of our "for()" statements use the value
   of the variable after iteration, for which this new construct
   cannot be used, though.

 - This may be showing I am not just old fashioned but also am
   ignorant, but I do not see much point in using the following in
   our codebase (iow, I am not aware of places in the existing code
   that they can be improved by employing these features):

   . // comments
   . restricted pointers
   . static and type qualifiers in parameter array declarators



-- >8 --
Subject: [PATCH] clean.c: use designated initializer

This is another test balloon to see if we get complaints from people
whose compilers do not support designated initializer for arrays.

The use of the feature is not all that interesting for cases like
the one this patch touches, where the initialized elements of the
array is dense, but it would be nice if we can use the feature to
initialize an array that has elements initialized to interesting
values only sparsely.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/clean.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/builtin/clean.c b/builtin/clean.c
index 057fc97fe4..858df2c4ee 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -33,15 +33,6 @@ static const char *msg_skip_git_dir = N_("Skipping repository %s\n");
 static const char *msg_would_skip_git_dir = N_("Would skip repository %s\n");
 static const char *msg_warn_remove_failed = N_("failed to remove %s");
 
-static int clean_use_color = -1;
-static char clean_colors[][COLOR_MAXLEN] = {
-	GIT_COLOR_RESET,
-	GIT_COLOR_NORMAL,	/* PLAIN */
-	GIT_COLOR_BOLD_BLUE,	/* PROMPT */
-	GIT_COLOR_BOLD,		/* HEADER */
-	GIT_COLOR_BOLD_RED,	/* HELP */
-	GIT_COLOR_BOLD_RED,	/* ERROR */
-};
 enum color_clean {
 	CLEAN_COLOR_RESET = 0,
 	CLEAN_COLOR_PLAIN = 1,
@@ -51,6 +42,16 @@ enum color_clean {
 	CLEAN_COLOR_ERROR = 5
 };
 
+static int clean_use_color = -1;
+static char clean_colors[][COLOR_MAXLEN] = {
+	[CLEAN_COLOR_RESET] = GIT_COLOR_RESET,
+	[CLEAN_COLOR_PLAIN] = GIT_COLOR_NORMAL,
+	[CLEAN_COLOR_PROMPT] = GIT_COLOR_BOLD_BLUE,
+	[CLEAN_COLOR_HEADER] = GIT_COLOR_BOLD,
+	[CLEAN_COLOR_HELP] = GIT_COLOR_BOLD_RED,
+	[CLEAN_COLOR_ERROR] = GIT_COLOR_BOLD_RED,
+};
+
 #define MENU_OPTS_SINGLETON		01
 #define MENU_OPTS_IMMEDIATE		02
 #define MENU_OPTS_LIST_ONLY		04
-- 
2.14.0-rc0-148-g5448d1895b

Re: [PATCH] strbuf: use designated initializers in STRBUF_INIT

[Stefan Beller]

On Fri, Jul 14, 2017 at 9:11 AM, Junio C Hamano <gitster@pobox.com> wrote:

>  - I prefer to keep decl-after-statement out of our codebase.  I
>    view it as a big plus in code-readability to be able to see a
>    complete list of variables that will be used in a block upfront
>    before starting to read the code that uses them.

sounds good to me.

>  - Corollary to the above, I do not mind to have a variable
>    declaration in the initialization clause of a for() statement
>    (e.g. "for (int i = 0; i < 4; i++) { ... }"), as the scoping rule
>    is very sensible.  Some of our "for()" statements use the value
>    of the variable after iteration, for which this new construct
>    cannot be used, though.

I might send a test balloon for this.

>    . // comments

I would think these are useful for two reasons:
(a) these seem to be used widely outside of these old-style project
  (git, gcc, kernel), such that most new contributors need to be told
  to avoid these which adds to the entry burden.
(b) recursive nesting of comments is possible. We may not need this
  in the final patch, but I sure use that in development. To comment out
  a whole function containing multiple comments I have to select all
  lines and prefix them with // currently. If all comments were //, I could
  put /* .. */ around the function, which seems easier. I just realize
  I can use #if 0 .. #endif though.

  (a) may be a concern, (b) not so much from the projects point of view.

>    . restricted pointers

That sounds like the ultimate micro optimization, but may be hard
to reason about after a years of churn, so it may become hard to
maintain.

>    . static and type qualifiers in parameter array declarators

That sounds equally arcane.

> -- >8 --
> Subject: [PATCH] clean.c: use designated initializer
>
> This is another test balloon to see if we get complaints from people
> whose compilers do not support designated initializer for arrays.

This sounds as if it is applied on top of Jeffs test balloon patch, such
that we do not need to re-iterate the criteria why to do it here, e.g.
this code is always compiled and did not change a lot over the last
couple month, so potentially easy to revert.

Thanks,
Stefan

Re: [PATCH] strbuf: use designated initializers in STRBUF_INIT

[Jeff King]

On Fri, Jul 14, 2017 at 09:11:33AM -0700, Junio C Hamano wrote:

> As to other things that we currently not allow in our codebase that
> newer compilers can grok, here is what *I* think.  It is *not* meant
> to be an exhaustive "what's new in C99 that is not in C89? what is
> the final verdict on each of them?":
> 
>  - There were occasional cases where we wished if variable-length
>    arrays, flexible array members and variadic macros were available
>    in our codebase during the course of this project.  We would
>    probably want to add a similar test baloon patch for each of
>    them to this series that is currently two-patch long.

I think variable-length arrays are potentially dangerous. They're
allocated on the stack, which creates two issues:

  1. You can run out of stack space and segfault, whereas the same
     operation with a heap buffer would be fine. You can say "but this
     VLA will only be used for small things". But then, you can just as
     easily declare a small stack buffer.

  2. My understanding of the recent "Stack Clash" class of
     vulnerabilities[1] is that VLAs make the attacker's job much easier
     (since they can often just send a large input to get you to
     allocate a large stack).

I think variadic macros are a good candidate, though. There have been a
number of times where we've had to sacrifice functionality or
readability in our helper functions. E.g., the case mentioned in
368953912 (add helpers for allocating flex-array structs, 2016-02-22).

The weather-balloon patch for that should be easy, too: just drop the
fallback macros from BUG() or the trace code.

[1] https://www.qualys.com/2017/06/19/stack-clash/stack-clash.txt

>  - I prefer to keep decl-after-statement out of our codebase.  I
>    view it as a big plus in code-readability to be able to see a
>    complete list of variables that will be used in a block upfront
>    before starting to read the code that uses them.
> 
>  - Corollary to the above, I do not mind to have a variable
>    declaration in the initialization clause of a for() statement
>    (e.g. "for (int i = 0; i < 4; i++) { ... }"), as the scoping rule
>    is very sensible.  Some of our "for()" statements use the value
>    of the variable after iteration, for which this new construct
>    cannot be used, though.

I agree with both of those points. I think the decl-in-for is nice
exactly because it highlights those cases where the iteration variable's
value is relevant after the loop ends.

>  - This may be showing I am not just old fashioned but also am
>    ignorant, but I do not see much point in using the following in
>    our codebase (iow, I am not aware of places in the existing code
>    that they can be improved by employing these features):
> 
>    . // comments
>    . restricted pointers
>    . static and type qualifiers in parameter array declarators

Agreed, though I think the comment thing is a personal taste issue (just
not my taste).

> +static int clean_use_color = -1;
> +static char clean_colors[][COLOR_MAXLEN] = {
> +	[CLEAN_COLOR_RESET] = GIT_COLOR_RESET,
> +	[CLEAN_COLOR_PLAIN] = GIT_COLOR_NORMAL,
> +	[CLEAN_COLOR_PROMPT] = GIT_COLOR_BOLD_BLUE,
> +	[CLEAN_COLOR_HEADER] = GIT_COLOR_BOLD,
> +	[CLEAN_COLOR_HELP] = GIT_COLOR_BOLD_RED,
> +	[CLEAN_COLOR_ERROR] = GIT_COLOR_BOLD_RED,
> +};

I think this is much nicer to read. I assume if we have a "hole" in our
numbering that the hole is initialized in the usual static way (a
COLOR_MAXLEN array full of NULs in this case, I guess)?

-Peff

Re: [PATCH] strbuf: use designated initializers in STRBUF_INIT

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> +static int clean_use_color = -1;
>> +static char clean_colors[][COLOR_MAXLEN] = {
>> +	[CLEAN_COLOR_RESET] = GIT_COLOR_RESET,
>> +	[CLEAN_COLOR_PLAIN] = GIT_COLOR_NORMAL,
>> +	[CLEAN_COLOR_PROMPT] = GIT_COLOR_BOLD_BLUE,
>> +	[CLEAN_COLOR_HEADER] = GIT_COLOR_BOLD,
>> +	[CLEAN_COLOR_HELP] = GIT_COLOR_BOLD_RED,
>> +	[CLEAN_COLOR_ERROR] = GIT_COLOR_BOLD_RED,
>> +};
>
> I think this is much nicer to read. I assume if we have a "hole" in our
> numbering that the hole is initialized in the usual static way (a
> COLOR_MAXLEN array full of NULs in this case, I guess)?

I would expect that would be the case.  

Do we need to have a check to detect a buggy compiler that takes the
syntax but produces an incorrectly initialized array?  I could add a
test to ensure that HEADER comes out BOLD, etc. (or we may already
have such a test) and then reorder these lines in this patch, if
that is the kind of breakage we anticipate.

Re: [PATCH] strbuf: use designated initializers in STRBUF_INIT

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Do we need to have a check to detect a buggy compiler that takes the
> syntax but produces an incorrectly initialized array?  I could add a
> test to ensure that HEADER comes out BOLD, etc. (or we may already
> have such a test) and then reorder these lines in this patch, if
> that is the kind of breakage we anticipate.

So here is a lunch-time hack I did to replace the patch I sent
earlier.  I kind of like the ordering of the elements better than
the original, in that it somehow feels more logical, even though I
merely sorted alphabetically ;-).


 builtin/clean.c              | 19 ++++++++++---------
 t/t7301-clean-interactive.sh | 10 ++++++++++
 2 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/builtin/clean.c b/builtin/clean.c
index 057fc97fe4..e2bb3c69ed 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -33,15 +33,6 @@ static const char *msg_skip_git_dir = N_("Skipping repository %s\n");
 static const char *msg_would_skip_git_dir = N_("Would skip repository %s\n");
 static const char *msg_warn_remove_failed = N_("failed to remove %s");
 
-static int clean_use_color = -1;
-static char clean_colors[][COLOR_MAXLEN] = {
-	GIT_COLOR_RESET,
-	GIT_COLOR_NORMAL,	/* PLAIN */
-	GIT_COLOR_BOLD_BLUE,	/* PROMPT */
-	GIT_COLOR_BOLD,		/* HEADER */
-	GIT_COLOR_BOLD_RED,	/* HELP */
-	GIT_COLOR_BOLD_RED,	/* ERROR */
-};
 enum color_clean {
 	CLEAN_COLOR_RESET = 0,
 	CLEAN_COLOR_PLAIN = 1,
@@ -51,6 +42,16 @@ enum color_clean {
 	CLEAN_COLOR_ERROR = 5
 };
 
+static int clean_use_color = -1;
+static char clean_colors[][COLOR_MAXLEN] = {
+	[CLEAN_COLOR_ERROR] = GIT_COLOR_BOLD_RED,
+	[CLEAN_COLOR_HEADER] = GIT_COLOR_BOLD,
+	[CLEAN_COLOR_HELP] = GIT_COLOR_BOLD_RED,
+	[CLEAN_COLOR_PLAIN] = GIT_COLOR_NORMAL,
+	[CLEAN_COLOR_PROMPT] = GIT_COLOR_BOLD_BLUE,
+	[CLEAN_COLOR_RESET] = GIT_COLOR_RESET,
+};
+
 #define MENU_OPTS_SINGLETON		01
 #define MENU_OPTS_IMMEDIATE		02
 #define MENU_OPTS_LIST_ONLY		04
diff --git a/t/t7301-clean-interactive.sh b/t/t7301-clean-interactive.sh
index 3ae394e934..556e1850e2 100755
--- a/t/t7301-clean-interactive.sh
+++ b/t/t7301-clean-interactive.sh
@@ -472,4 +472,14 @@ test_expect_success 'git clean -id with prefix and path (ask)' '
 
 '
 
+test_expect_success 'git clean -i paints the header in HEADER color' '
+	>a.out &&
+	echo q |
+	git -c color.ui=always clean -i |
+	test_decode_color |
+	head -n 1 >header &&
+	# not i18ngrep
+	grep "^<BOLD>" header
+'
+
 test_done

Re: [PATCH] strbuf: use designated initializers in STRBUF_INIT

[Ævar Arnfjörð Bjarmason]

On Fri, Jul 14 2017, Junio C. Hamano jotted:

> Junio C Hamano <gitster@pobox.com> writes:
>
>  - This may be showing I am not just old fashioned but also am
>    ignorant, but I do not see much point in using the following in
>    our codebase (iow, I am not aware of places in the existing code
>    that they can be improved by employing these features):
>
>    . // comments

[Feel free to ignore this E-Mail and my fascination with C syntax
trivia]

I wouldn't advocate switching to them on this basis, but since you asked
for cases where things could be improved with // comments:

The other day I submitted a patch that included this line in a comment:

    +        * "t/**.sh" and then conclude that there's a directory "t",

Which you fixed up to, before integrating it:

    +        * "t/" + "**.sh" and then conclude that there's a directory "t",

That was only necessary due to limitations in one of two comment
syntaxes modern C supports.

Well, it wasn't *necessary*, but a compiler warned about the /* there as
a possibly confusing construct, and any compiler would have ended the
comment right there + errored out if it contained "t/**/*.sh".

Re: [PATCH] strbuf: use designated initializers in STRBUF_INIT

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> On Fri, Jul 14 2017, Junio C. Hamano jotted:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>  - This may be showing I am not just old fashioned but also am
>>    ignorant, but I do not see much point in using the following in
>>    our codebase (iow, I am not aware of places in the existing code
>>    that they can be improved by employing these features):
>>
>>    . // comments
>
> [Feel free to ignore this E-Mail and my fascination with C syntax
> trivia]
>
> I wouldn't advocate switching to them on this basis, but since you asked
> for cases where things could be improved with // comments:
>
> The other day I submitted a patch that included this line in a comment:
>
>     +        * "t/**.sh" and then conclude that there's a directory "t",

Yes, obviously I am very aware of this one.  

The thing is, I had to do this only once in the 10+ year since
project inception.  I find that much more relevant anecdata ;-).

Re: [PATCH] strbuf: use designated initializers in STRBUF_INIT

[Mike Hommey]

On Fri, Jul 14, 2017 at 09:11:33AM -0700, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Oh, absolutely.
> >
> > Here is another possible test balloon, that may actually be useful
> > as an example.  I think there is a topic in flight that touches this
> > array, unfortunately, so I probably would find another one that is
> > more stable for a real follow-up patch to the one from Peff.
> 
> And here it is.
> 
> As to other things that we currently not allow in our codebase that
> newer compilers can grok, here is what *I* think.  It is *not* meant
> to be an exhaustive "what's new in C99 that is not in C89? what is
> the final verdict on each of them?":
> 
>  - There were occasional cases where we wished if variable-length
>    arrays, flexible array members and variadic macros were available
>    in our codebase during the course of this project.  We would
>    probably want to add a similar test baloon patch for each of
>    them to this series that is currently two-patch long.

FWIW, variadic macros have subtle implementation differences that can
cause problems.
For instance, MSVC only supports ##__VA_ARGS__ by way of
ignoring ## somehow, but has the default behavior of dropping the comma
when __VA_ARGS__ is empty, which means , __VA_ARGS__ *without* ## has a
different behavior.
See also https://connect.microsoft.com/VisualStudio/feedback/details/380090/variadic-macro-replacement

Mike

Re: [PATCH] strbuf: use designated initializers in STRBUF_INIT

[Jeff King]

On Sat, Jul 15, 2017 at 07:43:09AM +0900, Mike Hommey wrote:

> >  - There were occasional cases where we wished if variable-length
> >    arrays, flexible array members and variadic macros were available
> >    in our codebase during the course of this project.  We would
> >    probably want to add a similar test baloon patch for each of
> >    them to this series that is currently two-patch long.
> 
> FWIW, variadic macros have subtle implementation differences that can
> cause problems.
> For instance, MSVC only supports ##__VA_ARGS__ by way of
> ignoring ## somehow, but has the default behavior of dropping the comma
> when __VA_ARGS__ is empty, which means , __VA_ARGS__ *without* ## has a
> different behavior.
> See also https://connect.microsoft.com/VisualStudio/feedback/details/380090/variadic-macro-replacement

When there's any other non-optional argument, you can work around this
by just lumping that argument in with the varargs. So don't do:

  #define FOO(fmt, ...) printf(fmt, __VA_ARGS__)

which fails when the format has no placeholders.  Instead do:

  #define FOO(...) printf(__VA_ARGS__)

That limits your interface for variadic macros, but in practice it
doesn't usually come up (there are also disgusting macro tricks you can
do to cover the true 0-arg case portably, but we haven't had to turn to
them yet).

-Peff

[PATCH] objects: scope count variable to loop

[Stefan Beller]

This is another test balloon to see if we get complaints from people
whose compilers do not support variables scoped to for loops.

This part of the code base was chosen as it is very old code that does
not change often, such that a potential revert is easy.

Signed-off-by: Stefan Beller <sbeller@google.com>
---

This is a rather aggressive test ballon, my compiler needed some
good arguments to accept the new world order:

object.c: In function ‘object_array_remove_duplicates’:
object.c:404:2: error: ‘for’ loop initial declarations are only allowed in C99 mode
  for (unsigned src = 0; src < nr; src++) {
  ^
object.c:404:2: note: use option -std=c99 or -std=gnu99 to compile your code

Using -std=c99 works for me.

Thanks,
Stefan

 object.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/object.c b/object.c
index f818777412..af26ee2fbc 100644
--- a/object.c
+++ b/object.c
@@ -397,11 +397,11 @@ static int contains_name(struct object_array *array, const char *name)
 
 void object_array_remove_duplicates(struct object_array *array)
 {
-	unsigned nr = array->nr, src;
+	unsigned nr = array->nr;
 	struct object_array_entry *objects = array->objects;
 
 	array->nr = 0;
-	for (src = 0; src < nr; src++) {
+	for (unsigned src = 0; src < nr; src++) {
 		if (!contains_name(array, objects[src].name)) {
 			if (src != array->nr)
 				objects[array->nr] = objects[src];
-- 
2.14.0.rc0.3.g6c2e499285

Re: [PATCH] objects: scope count variable to loop

[Brandon Williams]

On 07/19, Stefan Beller wrote:
> This is another test balloon to see if we get complaints from people
> whose compilers do not support variables scoped to for loops.
> 
> This part of the code base was chosen as it is very old code that does
> not change often, such that a potential revert is easy.
> 
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> 
> This is a rather aggressive test ballon, my compiler needed some
> good arguments to accept the new world order:
> 
> object.c: In function ‘object_array_remove_duplicates’:
> object.c:404:2: error: ‘for’ loop initial declarations are only allowed in C99 mode
>   for (unsigned src = 0; src < nr; src++) {
>   ^
> object.c:404:2: note: use option -std=c99 or -std=gnu99 to compile your code
> 
> Using -std=c99 works for me.

This would need a change to the makefile then wouldn't it?

> 
> Thanks,
> Stefan
> 
>  object.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/object.c b/object.c
> index f818777412..af26ee2fbc 100644
> --- a/object.c
> +++ b/object.c
> @@ -397,11 +397,11 @@ static int contains_name(struct object_array *array, const char *name)
>  
>  void object_array_remove_duplicates(struct object_array *array)
>  {
> -	unsigned nr = array->nr, src;
> +	unsigned nr = array->nr;
>  	struct object_array_entry *objects = array->objects;
>  
>  	array->nr = 0;
> -	for (src = 0; src < nr; src++) {
> +	for (unsigned src = 0; src < nr; src++) {
>  		if (!contains_name(array, objects[src].name)) {
>  			if (src != array->nr)
>  				objects[array->nr] = objects[src];
> -- 
> 2.14.0.rc0.3.g6c2e499285
> 

-- 
Brandon Williams

Re: [PATCH] objects: scope count variable to loop

[Jeff King]

On Wed, Jul 19, 2017 at 11:23:42AM -0700, Brandon Williams wrote:

> > object.c: In function ‘object_array_remove_duplicates’:
> > object.c:404:2: error: ‘for’ loop initial declarations are only allowed in C99 mode
> >   for (unsigned src = 0; src < nr; src++) {
> >   ^
> > object.c:404:2: note: use option -std=c99 or -std=gnu99 to compile your code
> > 
> > Using -std=c99 works for me.
> 
> This would need a change to the makefile then wouldn't it?

Actually, it complicates things even more, I'd think. We probably can't
just blindly add "-std=c99" to CFLAGS, as not all compilers would
support it (even if they _do_ support this construct).

Interestingly I have no problems compiling it here. I wonder if Stefan's
config.mak is supplying -std=c89 or some other restrictive flag. Or if
his compiler is a different version (though I tried with gcc-6, gcc-4.9,
and clang-3.8).

-Peff

Re: [PATCH] objects: scope count variable to loop

[Stefan Beller]

On Mon, Jul 24, 2017 at 10:08 AM, Jeff King <peff@peff.net> wrote:
> On Wed, Jul 19, 2017 at 11:23:42AM -0700, Brandon Williams wrote:
>
>> > object.c: In function ‘object_array_remove_duplicates’:
>> > object.c:404:2: error: ‘for’ loop initial declarations are only allowed in C99 mode
>> >   for (unsigned src = 0; src < nr; src++) {
>> >   ^
>> > object.c:404:2: note: use option -std=c99 or -std=gnu99 to compile your code
>> >
>> > Using -std=c99 works for me.
>>
>> This would need a change to the makefile then wouldn't it?
>
> Actually, it complicates things even more, I'd think. We probably can't
> just blindly add "-std=c99" to CFLAGS, as not all compilers would
> support it (even if they _do_ support this construct).
>
> Interestingly I have no problems compiling it here. I wonder if Stefan's
> config.mak is supplying -std=c89 or some other restrictive flag. Or if
> his compiler is a different version (though I tried with gcc-6, gcc-4.9,
> and clang-3.8).

Before this patch, I only had
  CFLAGS += -g -O0
in config.mak (as I switched working directories recently), I'll throw in
  DEVELOPER=1

My compiler version is ancient (gcc 4.8.4-2ubuntu1~14.04.3)
apparently (why did I never check in this environment?)

Re: [PATCH] objects: scope count variable to loop

[Jeff King]

On Mon, Jul 24, 2017 at 10:12:59AM -0700, Stefan Beller wrote:

> > Interestingly I have no problems compiling it here. I wonder if Stefan's
> > config.mak is supplying -std=c89 or some other restrictive flag. Or if
> > his compiler is a different version (though I tried with gcc-6, gcc-4.9,
> > and clang-3.8).
> 
> Before this patch, I only had
>   CFLAGS += -g -O0
> in config.mak (as I switched working directories recently), I'll throw in
>   DEVELOPER=1
> 
> My compiler version is ancient (gcc 4.8.4-2ubuntu1~14.04.3)
> apparently (why did I never check in this environment?)

Ah, indeed, it's the compiler version. And I actually screwed up my
gcc-4.9 test. It complains, too. It looks like the default for gcc
bumped from gnu90 to gnu11 in gcc 5.

-Peff