threads / patch / 29218

patch, 4 partsEliminate warnings under Sun Studio

Subject: [PATCH 0/4] Eliminate warnings under Sun Studio

## tl;dr

8 messages between Dec 21, 2011 and Dec 21, 2011. Diffs are folded; open one to read it.

replies: 7people: 2as markdown or json

Ævar Arnfjörð Bjarmason· Dec 21, 2011, 01:18 UTC · lore

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
Ævar Arnfjörð Bjarmason· Dec 21, 2011, 01:18 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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(-)
Show changes to builtin/fast-export.c +2 −2
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
Ævar Arnfjörð Bjarmason· Dec 21, 2011, 01:18 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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(-)
Show changes to 3 files +4 −4

test-treap.c, vcs-svn/repo_tree.c, vcs-svn/string_pool.c

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
Ævar Arnfjörð Bjarmason· Dec 21, 2011, 01:18 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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(-)
Show changes to 4 files +16 −16

builtin/index-pack.c, fast-import.c, pack-write.c, sha1_file.c

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
Ævar Arnfjörð Bjarmason· Dec 21, 2011, 01:18 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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(-)
Show changes to 2 files +12 −0

read-cache.c, xdiff/xutils.c

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
Junio C Hamano· Dec 21, 2011, 18:27 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
Show 17 quoted lines
> 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?

Show changes to read-cache.c +2 −2
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')
Ævar Arnfjörð Bjarmason· Dec 21, 2011, 19:03 UTC · re: Junio C Hamano · lore

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

On Wed, Dec 21, 2011 at 19:27, Junio C Hamano <gitster@pobox.com> wrote:
Show 44 quoted lines
> Æ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.

Junio C Hamano· Dec 21, 2011, 20:41 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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

← back to recent threads