Re: [PATCH] dir.c: avoid c99 array initialization

Brandon Casey [off-list ref] writes:

David Kågedal wrote:

quoted

Brandon Casey [off-list ref] writes:

quoted

The following syntax:

        char foo[] = {
                [0] = 1,
                [7] = 2,
                [15] = 3
        };

is a c99 construct which some compilers do not support even though they
support other c99 constructs. Use an alternative.

But the alternative is much worse.

_Much_ worse? In what way?

From an execution standpoint, I don't think any more work is performed.
Probably exactly the same amount of work.

From a readability standpoint, I think it is very nearly the same in
this case. The whole function is only 17 lines.

I do not think your patch deserves "much worse" comment.

quoted

So how important is it to support non-C99 compilers?

I think it is relative to the amount of effort it takes. If there is
a demonstrated need and a trivial work around, I think it is worth
it to support non-c99 compilers.

I do not mind taking this patch.  While it would not hurt (because the
code is readable with or without the change) to convert the trivial ones
like this patch addresses, it would not help portability if there are more
nontrivial dependance to c99 constructs in other places in the code.  Are
there known ones?

Junio C Hamano wrote:

I do not mind taking this patch.  While it would not hurt (because the
code is readable with or without the change) to convert the trivial ones
like this patch addresses, it would not help portability if there are more
nontrivial dependance to c99 constructs in other places in the code.  Are
there known ones?

Only one other "c99" issue that I have encountered (while using a c99 compiler
no less) and it is similarly trivial:

------->8-------
From: Brandon Casey <redacted>
Date: Wed, 13 Aug 2008 11:09:33 -0700
Subject: [PATCH] unpack-trees.c: work around run-time array initialization flaw on IRIX 6.5

The c99 MIPSpro Compiler version 7.4.4m on IRIX 6.5 does not properly
initialize run-time initialized arrays. An array which is initialized with
fewer elements than the length of the array should have the uninitialized
elements initialized to zero. This compiler does perform proper
initialization when static initialization parameters are used. So, work
around this by using compile time initialization.
---
 unpack-trees.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/unpack-trees.c b/unpack-trees.c
index cba0aca..de7cb0b 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c

@@ -143,7 +143,8 @@ static inline int call_unpack_fn(struct cache_entry **src, struct unpack_trees_o

 static int unpack_index_entry(struct cache_entry *ce, struct unpack_trees_options *o)
 {
-	struct cache_entry *src[5] = { ce, };
+	struct cache_entry *src[5] = { NULL, };
+	src[0] = ce;

 	o->pos++;
 	if (ce_stage(ce)) {

-- 
1.6.0.1.119.gcb7f
------->8-------


The above patch, along with minor changes to git-compat-util.h and a new entry in
Makefile allow me to compile on IRIX 6.5. (note: the dir.c patch is not necessary
on IRIX 6.5).

The dir.c patch along with two removals of the const modifier from option
structures and a few tweaks in Makefile allow me to compile using the SUNWspro
compiler on Solaris 7. I'm not sure if it is a c99 issue, but I get the following
errors when compiling without the following patches (which follow the errors):

"builtin-cat-file.c", line 121: warning: non-constant initializer: op "NAME"
"builtin-cat-file.c", line 217: warning: non-constant initializer: op "U&"
"builtin-cat-file.c", line 217: left operand must be modifiable lvalue: op "="
"builtin-cat-file.c", line 218: left operand must be modifiable lvalue: op "="
"builtin-cat-file.c", line 219: left operand must be modifiable lvalue: op "="
"builtin-cat-file.c", line 221: left operand must be modifiable lvalue: op "="
"builtin-cat-file.c", line 222: left operand must be modifiable lvalue: op "="
"builtin-cat-file.c", line 224: left operand must be modifiable lvalue: op "="
cc: acomp failed for builtin-cat-file.c

and similar errors for builtin-reset.c.

----->8-----
diff --git a/builtin-cat-file.c b/builtin-cat-file.c
index 7441a56..d954c09 100644
--- a/builtin-cat-file.c
+++ b/builtin-cat-file.c
@@ -212,7 +212,7 @@ int cmd_cat_file(int argc, const char **argv, const char *pr
 	int opt = 0, batch = 0;
 	const char *exp_type = NULL, *obj_name = NULL;
 
-	const struct option options[] = {
+	struct option options[] = {
 		OPT_GROUP("<type> can be one of: blob, tree, commit, tag"),
 		OPT_SET_INT('t', NULL, &opt, "show object type", 't'),
 		OPT_SET_INT('s', NULL, &opt, "show object size", 's'),
diff --git a/builtin-reset.c b/builtin-reset.c
index 4d246c3..28b633f 100644
--- a/builtin-reset.c
+++ b/builtin-reset.c
@@ -176,7 +176,7 @@ int cmd_reset(int argc, const char **argv, const char *prefi
				*old_orig = NULL, sha1_old_orig[20];
 	struct commit *commit;
 	char *reflog_action, msg[1024];
-	const struct option options[] = {
+	struct option options[] = {
 		OPT_SET_INT(0, "mixed", &reset_type,
 						"reset HEAD and index", MIXED),
 		OPT_SET_INT(0, "soft", &reset_type, "reset only HEAD", SOFT),
 
----->8-----

Running the tests with ksh on these two platforms requires a work around for ksh's
unique handling of trap within a function (which I do not yet have a pretty solution
for).

-brandon

2008/8/28 Brandon Casey [off-list ref]:

#ifdef HAVE_CONST
#define gitconst const
#else
#define gitconst
#endif

-	const struct option options[] = {
+	gitconst struct option options[] = {

?

Alex Riesen wrote:

2008/8/28 Brandon Casey [off-list ref]:

quoted

diff --git a/builtin-cat-file.c b/builtin-cat-file.c
index 7441a56..d954c09 100644
--- a/builtin-cat-file.c
+++ b/builtin-cat-file.c

@@ -212,7 +212,7 @@ int cmd_cat_file(int argc, const char **argv, const char *pr
       int opt = 0, batch = 0;
       const char *exp_type = NULL, *obj_name = NULL;

-       const struct option options[] = {
+       struct option options[] = {
               OPT_GROUP("<type> can be one of: blob, tree, commit, tag"),

#ifdef HAVE_CONST
#define gitconst const
#else
#define gitconst
#endif

-	const struct option options[] = {
+	gitconst struct option options[] = {

?

The problem is not that the const modifier is unrecognized. You can see in
the above patch that the const modifier was not removed from exp_type or
obj_name.

I think the warning on line 217 is the most important where it complains
about a non-constant initializer, followed by complaints that the left
operand must be a modifiable lvalue.

I notice now that some functions have the 'struct option' and all of its
parameters declared static. I think this would satisfy the compiler, but
then the options would not be reinitialized if the function was called
again. Or are cmd_*() functions defined to only ever be called once?

-brandon