[PATCH 0/4] Eliminate warnings under Sun Studio

[PATCH 0/4] Eliminate warnings under Sun Studio

[Ævar Arnfjörð Bjarmason]

This patch series eliminates warnings under Sun Studio. The first two
patches address actual (but obviously minor) issues, the third is a
nit, and the fourth disables a warning Sun Studio gets wrong.

I'm not sure whether we want the verbose code needed in the forth to
disable warnings under specific compilers, but since it's a rare
enough case and saves people compiling the code from wondering about
it's it's probably warranted. It's a verbose way to get rid of it
though.

I've CC'd people involved in the code touched by the first two, but
the second two are generic enough that I've decided not to bother the
original authors.

Ævar Arnfjörð Bjarmason (4):
  Fix an enum assignment issue spotted by Sun Studio
  Fix a bitwise negation assignment issue spotted by Sun Studio
  Appease Sun Studio by renaming "tmpfile"
  Suppress "statement not reached" warnings under Sun Studio

 builtin/fast-export.c |    4 ++--
 builtin/index-pack.c  |    6 +++---
 fast-import.c         |    8 ++++----
 pack-write.c          |    6 +++---
 read-cache.c          |    6 ++++++
 sha1_file.c           |   12 ++++++------
 test-treap.c          |    2 +-
 vcs-svn/repo_tree.c   |    2 +-
 vcs-svn/string_pool.c |    4 ++--
 xdiff/xutils.c        |    6 ++++++
 10 files changed, 34 insertions(+), 22 deletions(-)

-- 
1.7.7.3

[PATCH 1/4] Fix an enum assignment issue spotted by Sun Studio

[Ævar Arnfjörð Bjarmason]

In builtin/fast-export.c we'd assign to variables of the
tag_of_filtered_mode enum type with constants defined for the
signed_tag_mode enum.

We'd get the intended value since both the value we were assigning
with and the one we actually wanted had the same positional within
their respective enums, but doing it this way makes no sense.

This issue was spotted by Sun Studio 12 Update 1:

    "builtin/fast-export.c", line 54: warning: enum type mismatch: op "=" (E_ENUM_TYPE_MISMATCH_OP)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/fast-export.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 9836e6b..08fed98 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -25,7 +25,7 @@ static const char *fast_export_usage[] = {
 
 static int progress;
 static enum { ABORT, VERBATIM, WARN, STRIP } signed_tag_mode = ABORT;
-static enum { ERROR, DROP, REWRITE } tag_of_filtered_mode = ABORT;
+static enum { ERROR, DROP, REWRITE } tag_of_filtered_mode = ERROR;
 static int fake_missing_tagger;
 static int use_done_feature;
 static int no_data;
@@ -51,7 +51,7 @@ static int parse_opt_tag_of_filtered_mode(const struct option *opt,
 					  const char *arg, int unset)
 {
 	if (unset || !strcmp(arg, "abort"))
-		tag_of_filtered_mode = ABORT;
+		tag_of_filtered_mode = ERROR;
 	else if (!strcmp(arg, "drop"))
 		tag_of_filtered_mode = DROP;
 	else if (!strcmp(arg, "rewrite"))
-- 
1.7.7.3

[PATCH 2/4] Fix a bitwise negation assignment issue spotted by Sun Studio

[Ævar Arnfjörð Bjarmason]

Change direct and indirect assignments of the bitwise negation of 0 to
uint32_t variables to have a "U" suffix. I.e. ~0U instead of ~0. This
eliminates warnings under Sun Studio 12 Update 1:

    "vcs-svn/string_pool.c", line 11: warning: initializer will be sign-extended: -1 (E_INIT_SIGN_EXTEND)
    "vcs-svn/string_pool.c", line 81: warning: initializer will be sign-extended: -1 (E_INIT_SIGN_EXTEND)
    "vcs-svn/repo_tree.c", line 112: warning: initializer will be sign-extended: -1 (E_INIT_SIGN_EXTEND)
    "vcs-svn/repo_tree.c", line 112: warning: initializer will be sign-extended: -1 (E_INIT_SIGN_EXTEND)
    "test-treap.c", line 34: warning: initializer will be sign-extended: -1 (E_INIT_SIGN_EXTEND)

The semantics are still the same as demonstrated by this program:

    $ cat test.c && make test && ./test
    #include <stdio.h>
    #include <stdint.h>

    int main(void)
    {
        uint32_t foo = ~0;
        uint32_t bar = ~0U;

        printf("foo = <%u> bar = <%u>\n", foo, bar);

        return 0;
    }
    cc     test.c   -o test
    "test.c", line 5: warning: initializer will be sign-extended: -1
    foo = <4294967295> bar = <4294967295>

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 test-treap.c          |    2 +-
 vcs-svn/repo_tree.c   |    2 +-
 vcs-svn/string_pool.c |    4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/test-treap.c b/test-treap.c
index ab8c951..294d7ee 100644
--- a/test-treap.c
+++ b/test-treap.c
@@ -31,7 +31,7 @@ static void strtonode(struct int_node *item, const char *s)
 int main(int argc, char *argv[])
 {
 	struct strbuf sb = STRBUF_INIT;
-	struct trp_root root = { ~0 };
+	struct trp_root root = { ~0U };
 	uint32_t item;
 
 	if (argc != 1)
diff --git a/vcs-svn/repo_tree.c b/vcs-svn/repo_tree.c
index a21d89d..c3f198d 100644
--- a/vcs-svn/repo_tree.c
+++ b/vcs-svn/repo_tree.c
@@ -109,7 +109,7 @@ static struct repo_dirent *repo_read_dirent(uint32_t revision,
 static void repo_write_dirent(const uint32_t *path, uint32_t mode,
 			      uint32_t content_offset, uint32_t del)
 {
-	uint32_t name, revision, dir_o = ~0, parent_dir_o = ~0;
+	uint32_t name, revision, dir_o = ~0U, parent_dir_o = ~0U;
 	struct repo_dir *dir;
 	struct repo_dirent *key;
 	struct repo_dirent *dent = NULL;
diff --git a/vcs-svn/string_pool.c b/vcs-svn/string_pool.c
index 8af8d54..1b63b19 100644
--- a/vcs-svn/string_pool.c
+++ b/vcs-svn/string_pool.c
@@ -8,7 +8,7 @@
 #include "obj_pool.h"
 #include "string_pool.h"
 
-static struct trp_root tree = { ~0 };
+static struct trp_root tree = { ~0U };
 
 struct node {
 	uint32_t offset;
@@ -78,7 +78,7 @@ void pool_print_seq(uint32_t len, uint32_t *seq, char delim, FILE *stream)
 uint32_t pool_tok_seq(uint32_t sz, uint32_t *seq, const char *delim, char *str)
 {
 	char *context = NULL;
-	uint32_t token = ~0;
+	uint32_t token = ~0U;
 	uint32_t length;
 
 	if (sz == 0)
-- 
1.7.7.3

[PATCH 3/4] Appease Sun Studio by renaming "tmpfile"

[Ævar Arnfjörð Bjarmason]

On Solaris the system headers define the "tmpfile" name, which'll
cause Git compiled with Sun Studio 12 Update 1 to whine about us
redefining the name:

    "pack-write.c", line 76: warning: name redefined by pragma redefine_extname declared static: tmpfile     (E_PRAGMA_REDEFINE_STATIC)
    "sha1_file.c", line 2455: warning: name redefined by pragma redefine_extname declared static: tmpfile    (E_PRAGMA_REDEFINE_STATIC)
    "fast-import.c", line 858: warning: name redefined by pragma redefine_extname declared static: tmpfile   (E_PRAGMA_REDEFINE_STATIC)
    "builtin/index-pack.c", line 175: warning: name redefined by pragma redefine_extname declared static: tmpfile    (E_PRAGMA_REDEFINE_STATIC)

Just renaming the "tmpfile" variable to "tmp_file" in the relevant
places is the easiest way to fix this.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/index-pack.c |    6 +++---
 fast-import.c        |    8 ++++----
 pack-write.c         |    6 +++---
 sha1_file.c          |   12 ++++++------
 4 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 98025da..af7dc37 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -172,10 +172,10 @@ static const char *open_pack_file(const char *pack_name)
 	if (from_stdin) {
 		input_fd = 0;
 		if (!pack_name) {
-			static char tmpfile[PATH_MAX];
-			output_fd = odb_mkstemp(tmpfile, sizeof(tmpfile),
+			static char tmp_file[PATH_MAX];
+			output_fd = odb_mkstemp(tmp_file, sizeof(tmp_file),
 						"pack/tmp_pack_XXXXXX");
-			pack_name = xstrdup(tmpfile);
+			pack_name = xstrdup(tmp_file);
 		} else
 			output_fd = open(pack_name, O_CREAT|O_EXCL|O_RDWR, 0600);
 		if (output_fd < 0)
diff --git a/fast-import.c b/fast-import.c
index 4b9c4b7..6cd19e5 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -855,15 +855,15 @@ static struct tree_content *dup_tree_content(struct tree_content *s)
 
 static void start_packfile(void)
 {
-	static char tmpfile[PATH_MAX];
+	static char tmp_file[PATH_MAX];
 	struct packed_git *p;
 	struct pack_header hdr;
 	int pack_fd;
 
-	pack_fd = odb_mkstemp(tmpfile, sizeof(tmpfile),
+	pack_fd = odb_mkstemp(tmp_file, sizeof(tmp_file),
 			      "pack/tmp_pack_XXXXXX");
-	p = xcalloc(1, sizeof(*p) + strlen(tmpfile) + 2);
-	strcpy(p->pack_name, tmpfile);
+	p = xcalloc(1, sizeof(*p) + strlen(tmp_file) + 2);
+	strcpy(p->pack_name, tmp_file);
 	p->pack_fd = pack_fd;
 	p->do_not_close = 1;
 	pack_file = sha1fd(pack_fd, p->pack_name);
diff --git a/pack-write.c b/pack-write.c
index de2bd01..ca9e63b 100644
--- a/pack-write.c
+++ b/pack-write.c
@@ -73,9 +73,9 @@ const char *write_idx_file(const char *index_name, struct pack_idx_entry **objec
 		f = sha1fd_check(index_name);
 	} else {
 		if (!index_name) {
-			static char tmpfile[PATH_MAX];
-			fd = odb_mkstemp(tmpfile, sizeof(tmpfile), "pack/tmp_idx_XXXXXX");
-			index_name = xstrdup(tmpfile);
+			static char tmp_file[PATH_MAX];
+			fd = odb_mkstemp(tmp_file, sizeof(tmp_file), "pack/tmp_idx_XXXXXX");
+			index_name = xstrdup(tmp_file);
 		} else {
 			unlink(index_name);
 			fd = open(index_name, O_CREAT|O_EXCL|O_WRONLY, 0600);
diff --git a/sha1_file.c b/sha1_file.c
index f291f3f..88f2151 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2452,15 +2452,15 @@ static int write_loose_object(const unsigned char *sha1, char *hdr, int hdrlen,
 	git_SHA_CTX c;
 	unsigned char parano_sha1[20];
 	char *filename;
-	static char tmpfile[PATH_MAX];
+	static char tmp_file[PATH_MAX];
 
 	filename = sha1_file_name(sha1);
-	fd = create_tmpfile(tmpfile, sizeof(tmpfile), filename);
+	fd = create_tmpfile(tmp_file, sizeof(tmp_file), filename);
 	if (fd < 0) {
 		if (errno == EACCES)
 			return error("insufficient permission for adding an object to repository database %s\n", get_object_directory());
 		else
-			return error("unable to create temporary sha1 filename %s: %s\n", tmpfile, strerror(errno));
+			return error("unable to create temporary sha1 filename %s: %s\n", tmp_file, strerror(errno));
 	}
 
 	/* Set it up */
@@ -2505,12 +2505,12 @@ static int write_loose_object(const unsigned char *sha1, char *hdr, int hdrlen,
 		struct utimbuf utb;
 		utb.actime = mtime;
 		utb.modtime = mtime;
-		if (utime(tmpfile, &utb) < 0)
+		if (utime(tmp_file, &utb) < 0)
 			warning("failed utime() on %s: %s",
-				tmpfile, strerror(errno));
+				tmp_file, strerror(errno));
 	}
 
-	return move_temp_to_file(tmpfile, filename);
+	return move_temp_to_file(tmp_file, filename);
 }
 
 int write_sha1_file(const void *buf, unsigned long len, const char *type, unsigned char *returnsha1)
-- 
1.7.7.3

[PATCH 4/4] Suppress "statement not reached" warnings under Sun Studio

[Ævar Arnfjörð Bjarmason]

Sun Studio 12 Update 1's brain will melt on these two occurances of
using "goto" to jump into a loop. It'll emit these warnings:

    "read-cache.c", line 761: warning: statement not reached (E_STATEMENT_NOT_REACHED)
    "xdiff/xutils.c", line 194: warning: statement not reached (E_STATEMENT_NOT_REACHED)

Suppress these warnings by using a Sun Studio specific pragma
directive to turn them off, but only do so if __sun is defined, which
is the macro Sun Studio uses to identify itself under both its C and
C++ variants, see http://developers.sun.com/sunstudio/products/faqs/cpp.html

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 read-cache.c   |    6 ++++++
 xdiff/xutils.c |    6 ++++++
 2 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index a51bba1..0a4e895 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -758,7 +758,13 @@ int verify_path(const char *path)
 		return 0;
 
 	goto inside;
+#ifdef __sun
+#	pragma error_messages (off, E_STATEMENT_NOT_REACHED)
+#endif
 	for (;;) {
+#ifdef __sun
+#	pragma error_messages (on, E_STATEMENT_NOT_REACHED)
+#endif
 		if (!c)
 			return 1;
 		if (is_dir_sep(c)) {
diff --git a/xdiff/xutils.c b/xdiff/xutils.c
index 0de084e..62c3567 100644
--- a/xdiff/xutils.c
+++ b/xdiff/xutils.c
@@ -191,7 +191,13 @@ int xdl_recmatch(const char *l1, long s1, const char *l2, long s2, long flags)
 	 */
 	if (flags & XDF_IGNORE_WHITESPACE) {
 		goto skip_ws;
+#ifdef __sun
+#	pragma error_messages (off, E_STATEMENT_NOT_REACHED)
+#endif
 		while (i1 < s1 && i2 < s2) {
+#ifdef __sun
+#	pragma error_messages (on, E_STATEMENT_NOT_REACHED)
+#endif
 			if (l1[i1++] != l2[i2++])
 				return 0;
 		skip_ws:
-- 
1.7.7.3

Re: [PATCH 4/4] Suppress "statement not reached" warnings under Sun Studio

[Junio C Hamano]

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

> diff --git a/read-cache.c b/read-cache.c
> index a51bba1..0a4e895 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -758,7 +758,13 @@ int verify_path(const char *path)
>  		return 0;
>  
>  	goto inside;
> +#ifdef __sun
> +#	pragma error_messages (off, E_STATEMENT_NOT_REACHED)
> +#endif
>  	for (;;) {
> +#ifdef __sun
> +#	pragma error_messages (on, E_STATEMENT_NOT_REACHED)
> +#endif
>  		if (!c)
>  			return 1;

Patches 1-3 makes sense, but this one is too ugly to live.

Wouldn't something like this be equivalent and have the same effect
without sacrificing the readablity?

diff --git a/read-cache.c b/read-cache.c
index a51bba1..73af797 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -757,12 +757,12 @@ int verify_path(const char *path)
 	if (has_dos_drive_prefix(path))
 		return 0;
 
-	goto inside;
+	/* we are at the beginning of a path component */
+	c = '/';
 	for (;;) {
 		if (!c)
 			return 1;
 		if (is_dir_sep(c)) {
-inside:
 			c = *path++;
 			if ((c == '.' && !verify_dotfile(path)) ||
 			    is_dir_sep(c) || c == '\0')

Re: [PATCH 4/4] Suppress "statement not reached" warnings under Sun Studio

[Ævar Arnfjörð Bjarmason]

On Wed, Dec 21, 2011 at 19:27, Junio C Hamano <gitster@pobox.com> wrote:
> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> diff --git a/read-cache.c b/read-cache.c
>> index a51bba1..0a4e895 100644
>> --- a/read-cache.c
>> +++ b/read-cache.c
>> @@ -758,7 +758,13 @@ int verify_path(const char *path)
>>               return 0;
>>
>>       goto inside;
>> +#ifdef __sun
>> +#    pragma error_messages (off, E_STATEMENT_NOT_REACHED)
>> +#endif
>>       for (;;) {
>> +#ifdef __sun
>> +#    pragma error_messages (on, E_STATEMENT_NOT_REACHED)
>> +#endif
>>               if (!c)
>>                       return 1;
>
> Patches 1-3 makes sense, but this one is too ugly to live.
>
> Wouldn't something like this be equivalent and have the same effect
> without sacrificing the readablity?
>
> diff --git a/read-cache.c b/read-cache.c
> index a51bba1..73af797 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -757,12 +757,12 @@ int verify_path(const char *path)
>        if (has_dos_drive_prefix(path))
>                return 0;
>
> -       goto inside;
> +       /* we are at the beginning of a path component */
> +       c = '/';
>        for (;;) {
>                if (!c)
>                        return 1;
>                if (is_dir_sep(c)) {
> -inside:
>                        c = *path++;
>                        if ((c == '.' && !verify_dotfile(path)) ||
>                            is_dir_sep(c) || c == '\0')

That would make that warning go away, but I don't know if that changes
the semantics of the code. I was aiming not to change any code, just
to squash spurious warnings under Sun Studio.

We could also just wrap the whole function definition in the pragma,
which would make the code more readable since we wouldn't have 6 lines
of warning suppression in the middle of the function.

Or we could just drop this patch entirely, or rewrite the code. Your
pick.

Re: [PATCH 4/4] Suppress "statement not reached" warnings under Sun Studio

[Junio C Hamano]

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

> That would make that warning go away, but I don't know if that changes
> the semantics of the code. I was aiming not to change any code, just
> to squash spurious warnings under Sun Studio.

Well, earlier we skipped "if it is NUL return 1 and otherwise make sure it
is a directory separator" check and went directly into whatever happens
after we see a directory separator. The patch causes the same without goto.

If the code is too complex to confuse not so bright compilers, it is
likely to confuse no so bright humans as well, and rewriting the logic in
a more straightforward way to help humans is independently a good thing.

I am not particularly interested in squashing spurious warnings, but if it
falls out of a side effect of helping humans, I wouldn't object to it.