[PATCH] Fix Solaris Workshop Compiler issues

[PATCH] Fix Solaris Workshop Compiler issues

[Guido Ostkamp]

Hello,

please find below a patch that solves an error when compiling with the 
original Sun Solaris Compiler. When compiling out of the box, the 
following happens:

     CC diff-delta.o
"diff-delta.c", line 314: identifier redeclared: create_delta
 	current : function(pointer to const struct delta_index {unsigned long memsize, pointer to const void src_buf, unsigned long src_size, unsigned int hash_mask, array[-1] of pointer to struct index_entry {..} hash}, pointer to const void, unsigned long, pointer to unsigned long, unsigned long) returning pointer to void
 	previous: function(pointer to const struct delta_index {unsigned long memsize, pointer to const void src_buf, unsigned long src_size, unsigned int hash_mask, array[-1] of pointer to struct index_entry {..} hash}, pointer to const void, unsigned long, pointer to unsigned long, unsigned long) returning pointer to void : "delta.h", line 44
cc: acomp failed for diff-delta.c
make: *** [diff-delta.o] Error 2

This is because 'struct delta_index' is declared with no size in delta.h 
and with size in diff-delta.c which does not fit.

When the struct definition is done in the header file as one would 
normally expect, everything compiles ok with exception of a 
mkdtemp()-issue which somebody else already took care of on this list.

Best regards

Guido


diff --git a/delta.h b/delta.h
index 40ccf5a..06af9a7 100644
--- a/delta.h
+++ b/delta.h
@@ -1,8 +1,23 @@
  #ifndef DELTA_H
  #define DELTA_H

-/* opaque object for delta index */
-struct delta_index;
+struct index_entry {
+    const unsigned char *ptr;
+    unsigned int val;
+};
+
+struct unpacked_index_entry {
+    struct index_entry entry;
+    struct unpacked_index_entry *next;
+};
+
+struct delta_index {
+    unsigned long memsize;
+    const void *src_buf;
+    unsigned long src_size;
+    unsigned int hash_mask;
+    struct index_entry *hash[FLEX_ARRAY];
+};

  /*
   * create_delta_index: compute index data from given buffer
diff --git a/diff-delta.c b/diff-delta.c
index 9e440a9..2023e40 100644
--- a/diff-delta.c
+++ b/diff-delta.c
@@ -112,24 +112,6 @@ static const unsigned int U[256] = {
  	0x133eb0ac, 0x6d8b90a1, 0x450d4467, 0x3bb8646a
  };

-struct index_entry {
-	const unsigned char *ptr;
-	unsigned int val;
-};
-
-struct unpacked_index_entry {
-	struct index_entry entry;
-	struct unpacked_index_entry *next;
-};
-
-struct delta_index {
-	unsigned long memsize;
-	const void *src_buf;
-	unsigned long src_size;
-	unsigned int hash_mask;
-	struct index_entry *hash[FLEX_ARRAY];
-};
-
  struct delta_index * create_delta_index(const void *buf, unsigned long bufsize)
  {
  	unsigned int i, hsize, hmask, entries, prev_val, *hash_count;

Re: [PATCH] Fix Solaris Workshop Compiler issues

[Alex Riesen]

Guido Ostkamp, Wed, Nov 14, 2007 21:31:13 +0100:
> Hello,
>
> please find below a patch that solves an error when compiling with the 
> original Sun Solaris Compiler. When compiling out of the box, the following 
> happens:
>
>     CC diff-delta.o
> "diff-delta.c", line 314: identifier redeclared: create_delta
> 	current : function(pointer to const struct delta_index {unsigned long 
> memsize, pointer to const void src_buf, unsigned long src_size, unsigned 
> int hash_mask, array[-1] of pointer to struct index_entry {..} hash}, 
> pointer to const void, unsigned long, pointer to unsigned long, unsigned 
> long) returning pointer to void
> 	previous: function(pointer to const struct delta_index {unsigned long 
> memsize, pointer to const void src_buf, unsigned long src_size, unsigned 
> int hash_mask, array[-1] of pointer to struct index_entry {..} hash}, 
> pointer to const void, unsigned long, pointer to unsigned long, unsigned 
> long) returning pointer to void : "delta.h", line 44

The both prototypes listed are *exactly* the same. And both are wrong.
Looks like you're dealing with typically broken Sun compiler.

Try defining const to nothing or removing it from this prototype.
I suspect the thing is just so old and broken that it does not even
know anything of const.

> cc: acomp failed for diff-delta.c
> make: *** [diff-delta.o] Error 2
>
> This is because 'struct delta_index' is declared with no size in delta.h 
> and with size in diff-delta.c which does not fit.

Huh?! Ever heard of forward declaration?

Re: [PATCH] Fix Solaris Workshop Compiler issues

[Junio C Hamano]

Alex Riesen <raa.lkml@gmail.com> writes:

> Guido Ostkamp, Wed, Nov 14, 2007 21:31:13 +0100:
>> ...
>> cc: acomp failed for diff-delta.c
>> make: *** [diff-delta.o] Error 2
>>
>> This is because 'struct delta_index' is declared with no size in delta.h 
>> and with size in diff-delta.c which does not fit.
>
> Huh?! Ever heard of forward declaration?

We are not the first people who pass around a pointer to an
opaque struct in the API to hide away the implementation.  It
would be surprising if the Workshop Compiler chokes on this and
not other projects.

Re: [PATCH] Fix Solaris Workshop Compiler issues

[Guido Ostkamp]

On Wed, 14 Nov 2007, Junio C Hamano wrote:

> Alex Riesen <raa.lkml@gmail.com> writes:
>
>> Guido Ostkamp, Wed, Nov 14, 2007 21:31:13 +0100:
>>> ...
>>> cc: acomp failed for diff-delta.c
>>> make: *** [diff-delta.o] Error 2
>>>
>>> This is because 'struct delta_index' is declared with no size in delta.h
>>> and with size in diff-delta.c which does not fit.
>>
>> Huh?! Ever heard of forward declaration?
>
> We are not the first people who pass around a pointer to an opaque 
> struct in the API to hide away the implementation.  It would be 
> surprising if the Workshop Compiler chokes on this and not other 
> projects.

You got the original error report from Sun's compiler included in my 
earlier email. This happens with at least Sun Forte 6.1 (Solaris 8) and 
Sun Workshop 11 (Solaris 10), IIRC.

The function declarations regarding create_delta() in delta.h and 
diff-delta.c are identical with respect to the type names of the parameter 
(only some internal names e.g. like 'buf' vs. 'trg_buf' are slightly 
different, but this has no effect).

The main difference is that the 'struct delta_index' is opaque in delta.h 
and non-opaque in diff-delta.c; the patch clearly shows it solves the 
error. So we've got a solution.

If you feel we could try something else, please let me know and I'll check 
it out.

Please keep me on CC, as I'm not subscribed to the list, thanks.

Regards

Guido

Re: [PATCH] Fix Solaris Workshop Compiler issues

[Alex Riesen]

Guido Ostkamp, Thu, Nov 15, 2007 00:21:55 +0100:
> The main difference is that the 'struct delta_index' is opaque in delta.h 
> and non-opaque in diff-delta.c; the patch clearly shows it solves the 
> error. So we've got a solution.

It is not the solution. How do you think the rest of Git compiles?

> If you feel we could try something else, please let me know and I'll check 
> it out.

Yes. Read the mail I sent before:

    Try removing the "const". Looks like that compiler is too stupid
    to understand it.

Re: [PATCH] Fix Solaris Workshop Compiler issues

[Björn Steinbrink]

On 2007.11.15 00:28:09 +0100, Alex Riesen wrote:
> Guido Ostkamp, Thu, Nov 15, 2007 00:21:55 +0100:
> > The main difference is that the 'struct delta_index' is opaque in delta.h 
> > and non-opaque in diff-delta.c; the patch clearly shows it solves the 
> > error. So we've got a solution.
> 
> It is not the solution. How do you think the rest of Git compiles?
> 
> > If you feel we could try something else, please let me know and I'll check 
> > it out.
> 
> Yes. Read the mail I sent before:
> 
>     Try removing the "const". Looks like that compiler is too stupid
>     to understand it.

No, just tried with cc: Sun C 5.7 Patch 117837-06 2005/10/05

It's the "struct hack", ie. the incomplete array at the end of
delta_index. Still looking for a fix/workaround.

Björn

Re: [PATCH] Fix Solaris Workshop Compiler issues

[Junio C Hamano]

Björn Steinbrink <B.Steinbrink@gmx.de> writes:

> No, just tried with cc: Sun C 5.7 Patch 117837-06 2005/10/05
>
> It's the "struct hack", ie. the incomplete array at the end of
> delta_index. Still looking for a fix/workaround.

Do you mean the "FLEX_ARRAY" thing?

You can ask for FLEX_ARRAY from the command line of your "make"
process.

There is this thing in git-compat-util.h

        #ifndef FLEX_ARRAY
        #if defined(__GNUC__) && (__GNUC__ < 3)
        #define FLEX_ARRAY 0
        #else
        #define FLEX_ARRAY /* empty */
        #endif
        #endif

The sources are written this way:

	struct foo {
        	... other members ...
                char last_member_that_is_flexible[FLEX_ARRAY];
	};

For older gcc, because we know about its lack of support, the
above turns into:

	struct foo {
        	... other members ...
                char last_member_that_is_flexible[0];
        }

But for recent enough compilers that grok the "flexible array
members", the above expands to:

	struct foo {
        	... other members ...
                char last_member_that_is_flexible[];
        }

Maybe your compiler needs -DFLEX_ARRAY=0 in CFLAGS?

Re: [PATCH] Fix Solaris Workshop Compiler issues

[Björn Steinbrink]

On 2007.11.14 16:30:08 -0800, Junio C Hamano wrote:
> Björn Steinbrink <B.Steinbrink@gmx.de> writes:
> 
> > No, just tried with cc: Sun C 5.7 Patch 117837-06 2005/10/05
> >
> > It's the "struct hack", ie. the incomplete array at the end of
> > delta_index. Still looking for a fix/workaround.
> 
> Do you mean the "FLEX_ARRAY" thing?
> 
> You can ask for FLEX_ARRAY from the command line of your "make"
> process.
> 
> There is this thing in git-compat-util.h
> 
>         #ifndef FLEX_ARRAY
>         #if defined(__GNUC__) && (__GNUC__ < 3)
>         #define FLEX_ARRAY 0
>         #else
>         #define FLEX_ARRAY /* empty */
>         #endif
>         #endif
> 
> The sources are written this way:
> 
> 	struct foo {
>         	... other members ...
>                 char last_member_that_is_flexible[FLEX_ARRAY];
> 	};
> 
> For older gcc, because we know about its lack of support, the
> above turns into:
> 
> 	struct foo {
>         	... other members ...
>                 char last_member_that_is_flexible[0];
>         }
> 
> But for recent enough compilers that grok the "flexible array
> members", the above expands to:
> 
> 	struct foo {
>         	... other members ...
>                 char last_member_that_is_flexible[];
>         }
> 
> Maybe your compiler needs -DFLEX_ARRAY=0 in CFLAGS?

Actually, I just created a test-case remotely on a Solaris box in my
university (see below) and didn't compile the actual git code. With the
FAM, cc complains about a redeclared identifier, with a zero-sized
array, it complains that an array cannot be zero-sized...

Seems to be a known bug in Sun Studio 10:
http://forum.java.sun.com/thread.jspa?threadID=5071896&messageID=9263771

Björn


#include <stdio.h>

struct foo;
void bar(const struct foo *, unsigned long);

struct bla {
	unsigned long foo;
};

struct foo {
	unsigned long val;
	struct bla *blas[];
};

void bar(const struct foo *foo, unsigned long val)
{
	printf("%lu %lu\n", foo->val, val);
}

int main()
{
	struct foo foo;
	foo.val = 10;
	bar(&foo, 20);

	return 0;
}

Re: [PATCH] Fix Solaris Workshop Compiler issues

[Junio C Hamano]

Björn Steinbrink <B.Steinbrink@gmx.de> writes:

> On 2007.11.14 16:30:08 -0800, Junio C Hamano wrote:
>
>> Maybe your compiler needs -DFLEX_ARRAY=0 in CFLAGS?
>
> Actually, I just created a test-case remotely on a Solaris box in my
> university (see below) and didn't compile the actual git code. With the
> FAM, cc complains about a redeclared identifier, with a zero-sized
> array, it complains that an array cannot be zero-sized...

I think you can pass -DFLEX_ARRAY=1 as a workaround.  It would
waste one array member in a flexible structure but that is
better than compiler choking.

Re: [PATCH] Fix Solaris Workshop Compiler issues

[Björn Steinbrink]

On 2007.11.14 16:46:20 -0800, Junio C Hamano wrote:
> Björn Steinbrink <B.Steinbrink@gmx.de> writes:
> 
> > On 2007.11.14 16:30:08 -0800, Junio C Hamano wrote:
> >
> >> Maybe your compiler needs -DFLEX_ARRAY=0 in CFLAGS?
> >
> > Actually, I just created a test-case remotely on a Solaris box in my
> > university (see below) and didn't compile the actual git code. With the
> > FAM, cc complains about a redeclared identifier, with a zero-sized
> > array, it complains that an array cannot be zero-sized...
> 
> I think you can pass -DFLEX_ARRAY=1 as a workaround.  It would
> waste one array member in a flexible structure but that is
> better than compiler choking.

Yeah, that at least compiles (didn't do any further tests), forgot to
say that in the last email.

Björn

[PATCH] Fix "identifier redeclared" compilation error with SUN cc.

[Björn Steinbrink]

Some versions of SUN's cc have a bug that causes them to complain about
a redeclared identifier when you use a function declaration that takes a
struct with a FAM and this struct has only been declared but not yet
defined.

IOW, this will fail:
	struct foo;

	void bar(struct foo *);

	struct foo {
		int v;
		long *a[];
	};

	void bar(struct foo *foo) {} // SUN cc bug strikes here

So when we detect a SUN cc, we use an array size of 1 to workaround that
bug.

Signed-off-by: Björn Steinbrink <B.Steinbrink@gmx.de>
---
Guido, could you please test this patch?

I have no clue which versions of SUN's cc are affected, so I simply enabled
the workaround for all versions. Someone with more knowledge about that
should probably limit the check to only do that for the broken versions.

 git-compat-util.h |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index ede9408..c3ff4b4 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -6,6 +6,8 @@
 #ifndef FLEX_ARRAY
 #if defined(__GNUC__) && (__GNUC__ < 3)
 #define FLEX_ARRAY 0
+#elif defined(sun) || defined(__SUN__)
+#define FLEX_ARRAY 1
 #else
 #define FLEX_ARRAY /* empty */
 #endif
-- 
1.5.3.5.643.g40e25

Re: [PATCH] Fix "identifier redeclared" compilation error with SUN cc.

[Guido Ostkamp]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 669 bytes --]

On Thu, 15 Nov 2007, Björn Steinbrink wrote:
> Some versions of SUN's cc have a bug that causes them to complain about
> a redeclared identifier when you use a function declaration that takes a
> struct with a FAM and this struct has only been declared but not yet
> defined.
> [...]
> Signed-off-by: Björn Steinbrink <B.Steinbrink@gmx.de>
> ---
> Guido, could you please test this patch?

I tested it and the file in question compiled ok for the two versions of 
Sun Forte and Workshop compilers that I have available.

I could also link & run the main git binary after applying my own 
mkdtemp() patch.

I hope your patch will get officially included.

Regards

Guido

Re: [PATCH] Fix "identifier redeclared" compilation error with SUN cc.

[Junio C Hamano]

Guido Ostkamp <git@ostkamp.fastmail.fm> writes:

> I tested it and the file in question compiled ok for the two versions
> of Sun Forte and Workshop compilers that I have available.
>
> I could also link & run the main git binary after applying my own
> mkdtemp() patch.

Are there problems with the implementation in compat/ directory,
we ship specifically to help platforms without mkdtemp()?

Re: [PATCH] Fix "identifier redeclared" compilation error with SUN cc.

[Guido Ostkamp]

On Thu, 15 Nov 2007, Junio C Hamano wrote:
> Are there problems with the implementation in compat/ directory, we ship 
> specifically to help platforms without mkdtemp()?

The Git version that I used for testing at office did not yet include your 
compat fix or did not activate it automatically.

I shall check this out tomorrow and let you know. Sorry, it's already late 
here in Germany (23:27h) - I need to get some sleep ;-)

Regards

Guido

[PATCH] Add mkdtemp() workaround for Sun Solaris 10

[Guido Ostkamp]

On Thu, 15 Nov 2007, Junio C Hamano wrote:
> Are there problems with the implementation in compat/ directory, we ship 
> specifically to help platforms without mkdtemp()?

I checked again and the answer is 'yes'. The reason is trivial - for 
Solaris 10 the workaround is not activated and my version of Solaris 10 
(Sparc) has no mkdtemp() in libc.so.

The following patch should fix this:

Activate mkdtemp() workaround for Solaris 10.

Signed-off-by: Guido Ostkamp <git@ostkamp.fastmail.fm>
---
  Makefile |    3 +++
  1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/Makefile b/Makefile
index e830bc7..9dc01df 100644
--- a/Makefile
+++ b/Makefile
@@ -431,6 +431,9 @@ ifeq ($(uname_S),SunOS)
  		NO_C99_FORMAT = YesPlease
  		NO_STRTOUMAX = YesPlease
  	endif
+	ifeq ($(uname_R),5.10)
+		NO_MKDTEMP = YesPlease
+	endif
  	INSTALL = ginstall
  	TAR = gtar
  	BASIC_CFLAGS += -D__EXTENSIONS__
-- 
1.5.3.5.721.g039b

[RFH] Solaris portability

[Junio C Hamano]

Guido Ostkamp <git@ostkamp.fastmail.fm> writes:

> On Thu, 15 Nov 2007, Junio C Hamano wrote:
>> Are there problems with the implementation in compat/ directory, we
>> ship specifically to help platforms without mkdtemp()?
>
> I checked again and the answer is 'yes'. The reason is trivial - for
> Solaris 10 the workaround is not activated and my version of Solaris
> 10 (Sparc) has no mkdtemp() in libc.so.

Thanks.

This makes me wonder if treating it just like strcasestr() might
be simpler.  Could folks with access to Solaris boxes of
different vintages please see if the attached patch makes sense?

Can we also unify UNSETENV, SETENV, C99_FORMAT and STRTOUMAX, by
the way? 


diff --git a/Makefile b/Makefile
index e830bc7..cabde81 100644
--- a/Makefile
+++ b/Makefile
@@ -412,26 +412,25 @@ endif
 ifeq ($(uname_S),SunOS)
 	NEEDS_SOCKET = YesPlease
 	NEEDS_NSL = YesPlease
 	SHELL_PATH = /bin/bash
 	NO_STRCASESTR = YesPlease
 	NO_MEMMEM = YesPlease
 	NO_HSTRERROR = YesPlease
+	NO_MKDTEMP = YesPlease
 	ifeq ($(uname_R),5.8)
 		NEEDS_LIBICONV = YesPlease
 		NO_UNSETENV = YesPlease
 		NO_SETENV = YesPlease
-		NO_MKDTEMP = YesPlease
 		NO_C99_FORMAT = YesPlease
 		NO_STRTOUMAX = YesPlease
 	endif
 	ifeq ($(uname_R),5.9)
 		NO_UNSETENV = YesPlease
 		NO_SETENV = YesPlease
-		NO_MKDTEMP = YesPlease
 		NO_C99_FORMAT = YesPlease
 		NO_STRTOUMAX = YesPlease
 	endif
 	INSTALL = ginstall
 	TAR = gtar
 	BASIC_CFLAGS += -D__EXTENSIONS__
 endif

Re: [RFH] Solaris portability

[Guido Ostkamp]

On Fri, 16 Nov 2007, Junio C Hamano wrote:
> This makes me wonder if treating it just like strcasestr() might be 
> simpler.  Could folks with access to Solaris boxes of different vintages 
> please see if the attached patch makes sense?

I think the patch makes sense as neither Solaris 8, 9 nor 10 supports 
mkdtemp().

> Can we also unify UNSETENV, SETENV, C99_FORMAT and STRTOUMAX, by the 
> way?

No.

I've just checked on our Solaris Sparc systems, and found that the 
C-library provides unsetenv(), setenv() and strtoumax() beginning with 
Solaris 10; also the 'man sprintf' page mentions the 'z' and 't' 
specifiers for printf (which is what is behind C99_FORMAT) only beginning 
with Solaris 10.

So workarounds are needed for all 4 cases for Solaris 8 and 9 but not 10.

Regards

Guido

Re: [RFH] Solaris portability

[Junio C Hamano]

Guido Ostkamp <git@ostkamp.fastmail.fm> writes:

> On Fri, 16 Nov 2007, Junio C Hamano wrote:
>> This makes me wonder if treating it just like strcasestr() might be
>> simpler.  Could folks with access to Solaris boxes of different
>> vintages please see if the attached patch makes sense?
>
> I think the patch makes sense as neither Solaris 8, 9 nor 10 supports
> mkdtemp().
>
>> Can we also unify UNSETENV, SETENV, C99_FORMAT and STRTOUMAX, by the
>> way?
>
> No.
>
> I've just checked on our Solaris Sparc systems, and found that the
> C-library provides unsetenv(), setenv() and strtoumax() beginning with
> Solaris 10; also the 'man sprintf' page mentions the 'z' and 't'
> specifiers for printf (which is what is behind C99_FORMAT) only
> beginning with Solaris 10.
>
> So workarounds are needed for all 4 cases for Solaris 8 and 9 but not 10.

Thank you very much for detailed information.

Re: [PATCH] Fix "identifier redeclared" compilation error with SUN cc.

[Junio C Hamano]

Björn Steinbrink <B.Steinbrink@gmx.de> writes:

> Guido, could you please test this patch?
>
> I have no clue which versions of SUN's cc are affected, so I simply enabled
> the workaround for all versions. Someone with more knowledge about that
> should probably limit the check to only do that for the broken versions.
>
>  git-compat-util.h |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)

> diff --git a/git-compat-util.h b/git-compat-util.h
> index ede9408..c3ff4b4 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -6,6 +6,8 @@
>  #ifndef FLEX_ARRAY
>  #if defined(__GNUC__) && (__GNUC__ < 3)
>  #define FLEX_ARRAY 0
> +#elif defined(sun) || defined(__SUN__)
> +#define FLEX_ARRAY 1
>  #else
>  #define FLEX_ARRAY /* empty */
>  #endif

This feels a bit too narrow and too broad at the same time,
doesn't it?

As I suspect there are other compilers that do not implement
flexible array members (so you cannot use "member[]") nor older
gcc extension of zero sized member (so you cannot use
"member[0]" either), this checking specifically for Sun is too
narrow.

On the other hand, as you said, this is too broad, because not
everybody may be using the SUN compiler on Sun, nor the version
that does not understand flexible array members.

But being broad should always be safer, albeit a bit wasteful.

How about doing it this way?

  # ifndef FLEX_ARRAY
  #   if defined(__GNUC__)
  #     if (__GNUC__ < 3)
  #       define FLEX_ARRAY 0
  #     else
  #       define FLEX_ARRAY /* empty */
  #     endif
  #   else
        /* more cases we know we can use 0 or empty can come here */
  #   endif
  # endif

  /* if still undefined, default to the safe, old fashioned way */
  # ifndef FLEX_ARRAY
  #   define FLEX_ARRAY 1
  # endif

The basic idea is:

 * The user (from Makefile command line, config.mak, or you
   could add autoconf test) can pass -DFLEX_ARRAY=... to specify
   exactly what should happen;

 * Otherwise, if we happen to know for sure that we can use "0"
   or "/* empty */" with the compiler, we define FLEX_ARRAY;
   currently we know such things for gcc.

 * For everybody else, we use safer default of "1".  IOW, if you
   know your compiler does not grok "/* empty */" nor "0", you
   do not have to do anything special but use the default case
   as everybody else.

Re: [PATCH] Fix "identifier redeclared" compilation error with SUN cc.

[Björn Steinbrink]

On 2007.11.15 20:58:55 -0800, Junio C Hamano wrote:
> How about doing it this way?
> 
>   # ifndef FLEX_ARRAY
>   #   if defined(__GNUC__)
>   #     if (__GNUC__ < 3)
>   #       define FLEX_ARRAY 0
>   #     else
>   #       define FLEX_ARRAY /* empty */
>   #     endif
>   #   else
>         /* more cases we know we can use 0 or empty can come here */
>   #   endif
>   # endif
> 
>   /* if still undefined, default to the safe, old fashioned way */
>   # ifndef FLEX_ARRAY
>   #   define FLEX_ARRAY 1
>   # endif

Yeah, looks a bit smarter.

> The basic idea is:
> 
>  * The user (from Makefile command line, config.mak, or you
>    could add autoconf test) can pass -DFLEX_ARRAY=... to specify
>    exactly what should happen;

Eeeek! He said autoconf! Waaah! ... Ehrm, sorry where was I?

>  * Otherwise, if we happen to know for sure that we can use "0"
>    or "/* empty */" with the compiler, we define FLEX_ARRAY;
>    currently we know such things for gcc.
> 
>  * For everybody else, we use safer default of "1".  IOW, if you
>    know your compiler does not grok "/* empty */" nor "0", you
>    do not have to do anything special but use the default case
>    as everybody else.

Yep, definitely better than my patch.

Björn

[PATCH] Fix "identifier redeclared" compilation error with SUN cc

[Guido Ostkamp]

Hello Junio,

On Thu, 15 Nov 2007, Junio C Hamano wrote:
> As I suspect there are other compilers that do not implement flexible 
> array members (so you cannot use "member[]") nor older gcc extension of 
> zero sized member (so you cannot use "member[0]" either), this checking 
> specifically for Sun is too narrow.
>
> On the other hand, as you said, this is too broad, because not everybody 
> may be using the SUN compiler on Sun, nor the version that does not 
> understand flexible array members.
>
> But being broad should always be safer, albeit a bit wasteful.
>
> How about doing it this way?

it looks ok on Solaris. I assembled the following patch from your posting, 
could you please include it?


Signed-off-by: Guido Ostkamp <git@ostkamp.fastmail.fm>
---
  git-compat-util.h |   11 ++++++++++-
  1 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index 276a437..97759fd 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -4,11 +4,20 @@
  #define _FILE_OFFSET_BITS 64

  #ifndef FLEX_ARRAY
-#if defined(__GNUC__) && (__GNUC__ < 3)
+#if defined(__GNUC__)
+#if (__GNUC__ < 3)
  #define FLEX_ARRAY 0
  #else
  #define FLEX_ARRAY /* empty */
  #endif
+#else
+/* more cases we know we can use 0 or empty can come here */
+#endif
+#endif
+
+/* if still undefined, default to the safe, old fashioned way */
+#ifndef FLEX_ARRAY
+#define FLEX_ARRAY 1
  #endif

  #define ARRAY_SIZE(x) (sizeof(x)/sizeof(x[0]))
-- 
1.5.3.6.728.gea559

Re: [PATCH] Fix "identifier redeclared" compilation error with SUN cc

[Junio C Hamano]

Guido Ostkamp <git@ostkamp.fastmail.fm> writes:

>> ...
>> How about doing it this way?
>
> it looks ok on Solaris. I assembled the following patch from your
> posting, could you please include it?
>
>
> Signed-off-by: Guido Ostkamp <git@ostkamp.fastmail.fm>

I knew it would work on Solaris with gcc and cc that do not
understand flexible array members, but I am a bit worried about
other environments, where flexible array members are properly
supported.  They've been happily using member[] but with the
patch they suddenly start wasting a cell.

But we should do this sooner rather than later to find out any
breakage, and give people on platforms with a cc that supports
flexible array members and care about wasted memory enough time
to send patches to support their compiler in the way similar to
how gcc is supported.

But I cannot use your message with whitespace-broken patch (note
"format=flawed") and insufficient commit log message, which
means I have to do this myself.  Not tonight...

Re: [PATCH] Fix "identifier redeclared" compilation error with SUN cc

[Guido Ostkamp]

Hello Junio,

On Tue, 20 Nov 2007, Junio C Hamano wrote:
> I knew it would work on Solaris with gcc and cc that do not understand 
> flexible array members, but I am a bit worried about other environments, 
> where flexible array members are properly supported.  They've been 
> happily using member[] but with the patch they suddenly start wasting a 
> cell.
>
> But we should do this sooner rather than later to find out any breakage, 
> and give people on platforms with a cc that supports flexible array 
> members and care about wasted memory enough time to send patches to 
> support their compiler in the way similar to how gcc is supported.
>
> But I cannot use your message with whitespace-broken patch (note 
> "format=flawed") and insufficient commit log message, which means I have 
> to do this myself.  Not tonight...

sorry for the whitespace-issues. I have attached the patch again with 
improved log message and will turn off format-flawed for this email.

Please let me know if this one is ok and feel free to fix it.

Log message starts here:

Fix "identifier redeclared" compilation error with SUN cc.

The problem is caused by incomplete arrays like

   struct foo {
     ...
     char last_member_that_is_flexible[];
   }

which cannot be handled by certain compilers.
The solution is to change the last member to either

     char last_member_that_is_flexible[0]

or

     char last_member_that_is_flexible[1]

as required. Currently GNU CC can handle [] format for 
version 3 and later. Earlier versions need [0].
Non-GNU compiler use the safe form [1].

Signed-off-by: Guido Ostkamp <git@ostkamp.fastmail.fm>
---
  git-compat-util.h |   11 ++++++++++-
  1 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index 276a437..97759fd 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -4,11 +4,20 @@
  #define _FILE_OFFSET_BITS 64

  #ifndef FLEX_ARRAY
-#if defined(__GNUC__) && (__GNUC__ < 3)
+#if defined(__GNUC__)
+#if (__GNUC__ < 3)
  #define FLEX_ARRAY 0
  #else
  #define FLEX_ARRAY /* empty */
  #endif
+#else
+/* more cases we know we can use 0 or empty can come here */
+#endif
+#endif
+
+/* if still undefined, default to the safe, old fashioned way */
+#ifndef FLEX_ARRAY
+#define FLEX_ARRAY 1
  #endif

  #define ARRAY_SIZE(x) (sizeof(x)/sizeof(x[0]))
-- 
1.5.3.6.728.gea559

Re: [PATCH] Fix "identifier redeclared" compilation error with SUN cc

[Guido Ostkamp]

> sorry for the whitespace-issues. I have attached the patch again with 
> improved log message and will turn off format-flawed for this email.

only for the list: I noticed another whitespace problem and have meanwhile 
sent the patch to Junio as true attachment of a private email because I do 
not know whether attachments are accepted on this list (most likely they 
are not). Sorry for the inconvenience.

Regards

Guido

Re: [PATCH] Fix "identifier redeclared" compilation error with SUN cc

[Martin Mares]

Hello!

>  #ifndef FLEX_ARRAY
> -#if defined(__GNUC__) && (__GNUC__ < 3)
> +#if defined(__GNUC__)
> +#if (__GNUC__ < 3)
>  #define FLEX_ARRAY 0
>  #else
>  #define FLEX_ARRAY /* empty */
>  #endif
> +#else
> +/* more cases we know we can use 0 or empty can come here */
> +#endif
> +#endif
> +
> +/* if still undefined, default to the safe, old fashioned way */
> +#ifndef FLEX_ARRAY
> +#define FLEX_ARRAY 1
>  #endif

Do we really want to use empty FLEX_ARRAY only for a new gcc? Shouldn't
we test for C99 instead (__STDC_VERSION__ >= 199901L) and only if it
isn't C99, choose between 0 and 1 depending on gccness of the compiler?

				Have a nice fortnight
-- 
Martin `MJ' Mares                          <mj@ucw.cz>   http://mj.ucw.cz/
Faculty of Math and Physics, Charles University, Prague, Czech Rep., Earth
"First they ignore you. Then they laugh at you. Then they fight you. Then you win." -- Gandhi

Re: [PATCH] Fix "identifier redeclared" compilation error with SUN cc

[Junio C Hamano]

Martin Mares <mj@ucw.cz> writes:

> Do we really want to use empty FLEX_ARRAY only for a new gcc? Shouldn't
> we test for C99 instead (__STDC_VERSION__ >= 199901L) and only if it
> isn't C99, choose between 0 and 1 depending on gccness of the compiler?

How about doing it this way?

-- >8 --
[PATCH] git-compat-util.h: auto-adjust to compiler support of FLEX_ARRAY a bit better

When declaring a structure with a flexible array member, instead
of defaulting to the c99 syntax for non-gnu compilers (which
burned people with older compilers), default to the traditional
and more portable "member[1]; /* more */" syntax.

At the same time, other c99 compilers should be able to take
advantage of the modern syntax to flexible array members without
being gcc.  Check __STDC_VERSION__ for that.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 git-compat-util.h |   22 ++++++++++++++++++----
 1 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index 276a437..454d25e 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -4,10 +4,24 @@
 #define _FILE_OFFSET_BITS 64
 
 #ifndef FLEX_ARRAY
-#if defined(__GNUC__) && (__GNUC__ < 3)
-#define FLEX_ARRAY 0
-#else
-#define FLEX_ARRAY /* empty */
+/*
+ * See if our compiler is known to support flexible array members.
+ */
+#if defined(__STDC_VERSION__) && (__STDC_VERSION__ >= 199901L)
+# define FLEX_ARRAY /* empty */
+#elif defined(__GNUC__)
+# if (__GNUC__ >= 3)
+#  define FLEX_ARRAY /* empty */
+# else
+#  define FLEX_ARRAY 0 /* older GNU extension */
+# endif
+#endif
+
+/*
+ * Otherwise, default to safer but a bit wasteful traditional style
+ */
+#ifndef FLEX_ARRAY
+# define FLEX_ARRAY 1
 #endif
 #endif
 
-- 
1.5.3.6.1797.g67f5d

Re: [PATCH] Fix "identifier redeclared" compilation error with SUN cc

[Martin Mares]

Hello!

> How about doing it this way?

Yes, this looks perfect.

				Have a nice fortnight
-- 
Martin `MJ' Mares                          <mj@ucw.cz>   http://mj.ucw.cz/
Faculty of Math and Physics, Charles University, Prague, Czech Rep., Earth
Hi! I'm a .signature virus! Copy me into your ~/.signature to help me spread!

Re: [PATCH] Fix Solaris Workshop Compiler issues

[Linus Torvalds]

On Wed, 14 Nov 2007, Junio C Hamano wrote:
> 
> Maybe your compiler needs -DFLEX_ARRAY=0 in CFLAGS?

Actually, for old pre-C99 compilers, you're probably better off using 
-DFLEX_ARRAY=1, since a zero-sized array could be considered bogus by 
some.

			Linus

Re: [PATCH] Fix Solaris Workshop Compiler issues

[David Kastrup]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Wed, 14 Nov 2007, Junio C Hamano wrote:
>> 
>> Maybe your compiler needs -DFLEX_ARRAY=0 in CFLAGS?
>
> Actually, for old pre-C99 compilers, you're probably better off using 
> -DFLEX_ARRAY=1, since a zero-sized array could be considered bogus by 
> some.

Is that supposed to work?  I would have thought that the only options
would be empty and 0.  I am pretty sure I have seen size calculations in
the deltifying code that would break badly using FLEX_ARRAY=1.  So _IFF_
-DFLEX_ARRAY=1 is supposed to be necessary for some compilers, I could
try seeing whether I find those locations again.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: [PATCH] Fix Solaris Workshop Compiler issues

[Linus Torvalds]

On Thu, 15 Nov 2007, David Kastrup wrote:
> 
> Is that supposed to work?  I would have thought that the only options
> would be empty and 0.

I think it should work, even though some things allocations will end up 
being a bit too large (ie anything that uses "sizeof()" will have that one 
unnecessary entry)

But no, I didn't actually test it.

		Linus

Re: [PATCH] Fix Solaris Workshop Compiler issues

[Junio C Hamano]

David Kastrup <dak@gnu.org> writes:

> ...  I am pretty sure I have seen size calculations in
> the deltifying code that would break badly using FLEX_ARRAY=1.  So _IFF_
> -DFLEX_ARRAY=1 is supposed to be necessary for some compilers, I could
> try seeing whether I find those locations again.

I do recall that I received a patch with an explicit member
elem[1] that is in fact used as a flexible array, foolishly
converted it to use FLEX_ARRAY and saw it mysteriously fail, and
realized what it was doing and reverted my changes, and applied
the patch as received.  IIRC it all happened before I pushed the
results out.  I unfortunately do not recall which area the patch
was about.