threads / patch / 34477

patchFix some sparse warnings

Subject: [PATCH] Fix some sparse warnings

## tl;dr

5 messages between Jul 18, 2013 and Jul 21, 2013. Diffs are folded; open one to read it.

replies: 4people: 4as markdown or json

Ramsay Jones· Jul 18, 2013, 20:25 UTC · lore

Sparse issues some "Using plain integer as NULL pointer" warnings. Each warning relates to the use of an '{0}' initialiser expression in the declaration of an 'struct object_info'. The first field of this structure has pointer type. Thus, in order to suppress these warnings, we replace the initialiser expression with '{NULL}'.

Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
---
This was built on the next branch.

ATB, Ramsay Jones

 sha1_file.c | 2 +-
 streaming.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)
Show changes to 2 files +2 −2

sha1_file.c, streaming.c

diff --git a/sha1_file.c b/sha1_file.c
index 4c2365f..e4ab0a4 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2440,7 +2440,7 @@ int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi)
 
 int sha1_object_info(const unsigned char *sha1, unsigned long *sizep)
 {
-	struct object_info oi = {0};
+	struct object_info oi = {NULL};
 
 	oi.sizep = sizep;
 	return sha1_object_info_extended(sha1, &oi);
diff --git a/streaming.c b/streaming.c
index cac282f..5710065 100644
--- a/streaming.c
+++ b/streaming.c
@@ -135,7 +135,7 @@ struct git_istream *open_istream(const unsigned char *sha1,
 				 struct stream_filter *filter)
 {
 	struct git_istream *st;
-	struct object_info oi = {0};
+	struct object_info oi = {NULL};
 	const unsigned char *real = lookup_replace_object(sha1);
 	enum input_source src = istream_source(real, type, &oi);
 

-- 
1.8.3
Jeff King· Jul 18, 2013, 20:36 UTC · re: Ramsay Jones · lore

Re: [PATCH] Fix some sparse warnings

On Thu, Jul 18, 2013 at 09:25:50PM +0100, Ramsay Jones wrote:
Show 7 quoted lines
> Sparse issues some "Using plain integer as NULL pointer" warnings.
> Each warning relates to the use of an '{0}' initialiser expression
> in the declaration of an 'struct object_info'. The first field of
> this structure has pointer type. Thus, in order to suppress these
> warnings, we replace the initialiser expression with '{NULL}'.
> 
> Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
Acked-by: Jeff King <peff@peff.net>

I thought at first we would need one more for the new callsite I added in my series, but we use memset() in that instance, so it is fine.

-Peff
Ramsay Jones· Jul 20, 2013, 19:26 UTC · re: Jeff King · lore

Re: [PATCH] Fix some sparse warnings

Jeff King wrote:
Show 14 quoted lines
> On Thu, Jul 18, 2013 at 09:25:50PM +0100, Ramsay Jones wrote:
> 
>> Sparse issues some "Using plain integer as NULL pointer" warnings.
>> Each warning relates to the use of an '{0}' initialiser expression
>> in the declaration of an 'struct object_info'. The first field of
>> this structure has pointer type. Thus, in order to suppress these
>> warnings, we replace the initialiser expression with '{NULL}'.
>>
>> Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
> 
> Acked-by: Jeff King <peff@peff.net>
> 
> I thought at first we would need one more for the new callsite I added
> in my series, but we use memset() in that instance, so it is fine.

On an almost unrelated note ... I am now getting the following sparse warnings:

    pack-revindex.c:105:23: warning: memset with byte count of 262144

This is a little annoying, since there is no way to turn this off. :( (which I consider a bug in sparse).

Sparse has special-case code to check calls to memset(), memcpy(), copy_to_user() and copy_from_user(). The code that checks the byte count argument looks like:

static void check_byte_count(struct instruction *insn, pseudo_t count)
{
        if (!count)
                return;
        if (count->type == PSEUDO_VAL) {
                long long val = count->value;
                if (val <= 0 || val > 100000)
                        warning(insn->pos, "%s with byte count of %lld",
                                show_ident(insn->func->sym->ident), val);
                return;
        }
        /* OK, we could try to do the range analysis here */
}
I will just ignore this for now. ;-)

ATB, Ramsay Jones

Jonathan Nieder· Jul 21, 2013, 17:39 UTC · re: Ramsay Jones · lore

Re: [PATCH] Fix some sparse warnings

Hi,
Ramsay Jones wrote:
Show 5 quoted lines
> Sparse issues some "Using plain integer as NULL pointer" warnings.
> Each warning relates to the use of an '{0}' initialiser expression
> in the declaration of an 'struct object_info'. The first field of
> this structure has pointer type. Thus, in order to suppress these
> warnings, we replace the initialiser expression with '{NULL}'.

I agree that this is worth solving but the fix feels like a move in the wrong direction. Before this patch, the initialization says "= {0}", which basically means "memset it to 0". Afterward, the initializer depends on the order of fields in the struct.

How about something like the following?
Show changes to diff +3 −2
diff --git i/cache.h w/cache.h
index f2915509..ba028b75 100644
--- i/cache.h
+++ w/cache.h
@@ -1124,6 +1124,7 @@ struct object_info {
 		} packed;
 	} u;
 };
+#define OBJECT_INFO_INIT { NULL, NULL, OI_CACHED, { { NULL, 0, 0 } } }
 extern int sha1_object_info_extended(const unsigned char *, struct object_info *);
 
 /* Dumb servers support */
diff --git i/sha1_file.c w/sha1_file.c
index 6baed676..2d812b8d 100644
--- i/sha1_file.c
+++ w/sha1_file.c
@@ -2421,7 +2421,7 @@ int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi)
 
 int sha1_object_info(const unsigned char *sha1, unsigned long *sizep)
 {
-	struct object_info oi = {0};
+	struct object_info oi = OBJECT_INFO_INIT;
 
 	oi.sizep = sizep;
 	return sha1_object_info_extended(sha1, &oi);
diff --git i/streaming.c w/streaming.c
index cac282f0..b4c40ad9 100644
--- i/streaming.c
+++ w/streaming.c
@@ -135,7 +135,7 @@ struct git_istream *open_istream(const unsigned char *sha1,
 				 struct stream_filter *filter)
 {
 	struct git_istream *st;
-	struct object_info oi = {0};
+	struct object_info oi = OBJECT_INFO_INIT;
 	const unsigned char *real = lookup_replace_object(sha1);
 	enum input_source src = istream_source(real, type, &oi);
Junio C Hamano· Jul 21, 2013, 20:58 UTC · re: Jonathan Nieder · lore

Re: [PATCH] Fix some sparse warnings

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 11 quoted lines
> How about something like the following?
>
> diff --git i/cache.h w/cache.h
> index f2915509..ba028b75 100644
> --- i/cache.h
> +++ w/cache.h
> @@ -1124,6 +1124,7 @@ struct object_info {
>  		} packed;
>  	} u;
>  };
> +#define OBJECT_INFO_INIT { NULL, NULL, OI_CACHED, { { NULL, 0, 0 } } }
There are quite a many hits from
	$ git grep '= { *NULL *};'

only some of which are "char *var[] = { NULL }", which is perfectly fine, but others are to initialise "struct foo" (or "foo_t" which is typedef'ed) that share the same "writing NULL means the initialiser knows the order of the fields".

I doubt if the above #define is a maintainable solution, and worse yet, thinking about the fact that such a macro is necessary only for a structure whose first field happens to be of a pointer type, it strongly suggests me that it is not conceptually the right thing to do.

Can't we have sparse fixed (or skip this specific warning) instead?

← back to recent threads