threads / patch / 37056

patchtrace.h: suppress some sparse warnings and errors

Subject: [PATCH] trace.h: suppress some sparse warnings and errors

## tl;dr

One message between Jul 3, 2014 and Jul 3, 2014. Diffs are folded; open one to read it.

replies: 0people: 1as markdown or json

Ramsay Jones· Jul 3, 2014, 22:54 UTC · lore

Commit 07896a5c ("trace: improve trace performance", 02-07-2014) added a 'trace_key' structure to the trace.h header file which provokes sparse to issue numerous (837) warnings and errors, like so:

        SP abspath.c
    trace.h:8:26: warning: duplicate const
    trace.h:10:29: error: dubious one-bit signed bitfield
    trace.h:11:28: error: dubious one-bit signed bitfield

In order to suppress the warning, we simply remove the redundant 'const' keyword in the declaration of the key field.

The bit-field errors are addressed by changing the declaration to use an 'unsigned int' type for each bit-field. Note that the C standard says that using anything other than _Bool, signed int and unsigned int for the type of a bit-field is implementation defined. (In addition, the signed-ness of the 'char' type is also implementation defined).

Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
---
Hi Karsten,

If you need to re-roll your 'kb/perf-trace' branch, could you please squash this (or something like it) into the patch which corresponds to commit 07896a5c. Thanks!

Note that, if you had intended to declare the key field as a 'constant pointer to const char', then that would be spelt like: 'const char *const key;' instead.

I suspect that most (but maybe not all) compilers support 'unsigned char' bit-field types, which _may_ result in using a byte-sized storage unit for the two bit-fields combined. However, because of the alignment requirements of the other fields, the sizeof(struct trace_key) is 12 for both versions of the struct ('unsigned int' vs 'unsigned char') on 32-bit Linux (for both gcc and clang).

If you turn up the compiler warning levels (-Wall -Wextra) then both gcc and clang complain about missing initialisers for the trailing structure fields. These fields will be default initialised to zero anyway, but it also doesn't hurt to be more explicit.

So, an alternative patch may look like this:
      |diff --git a/trace.h b/trace.h
      |index 74d7396..1a193bf 100644
      |--- a/trace.h
      |+++ b/trace.h
      |@@ -5,13 +5,13 @@
      | #include "strbuf.h"
      | 
      | struct trace_key {
      |-	const char const *key;
      |+	const char *const key;
      | 	int fd;
      |-	char initialized : 1;
      |-	char need_close : 1;
      |+	unsigned int initialized : 1;
      |+	unsigned int need_close : 1;
      | };
      | 
      |-#define TRACE_KEY_INIT(name) { "GIT_TRACE_" #name }
      |+#define TRACE_KEY_INIT(name) { "GIT_TRACE_" #name, 0, 0, 0 }
      | 
      | extern void trace_repo_setup(const char *prefix);
      | extern int trace_want(struct trace_key *key);

ATB, Ramsay Jones

 trace.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
Show changes to trace.h +3 −3
diff --git a/trace.h b/trace.h
index 74d7396..1bbb341 100644
--- a/trace.h
+++ b/trace.h
@@ -5,10 +5,10 @@
 #include "strbuf.h"
 
 struct trace_key {
-	const char const *key;
+	const char *key;
 	int fd;
-	char initialized : 1;
-	char need_close : 1;
+	unsigned int initialized : 1;
+	unsigned int need_close : 1;
 };
 
 #define TRACE_KEY_INIT(name) { "GIT_TRACE_" #name }
-- 
2.0.0

← back to recent threads