git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
* [PATCH] read-cache.c: fix a sparse warning
@ 2018-09-14 23:29 Ramsay Jones
  2018-09-16  7:17 ` Eric Sunshine
  0 siblings, 1 reply; 5+ messages in thread
From: Ramsay Jones @ 2018-09-14 23:29 UTC (permalink / raw)
  To: Ben Peart; +Cc: Junio C Hamano, GIT Mailing-list


At one time, the POSIX standard required the type used to represent
a thread handle (pthread_t) be an arithmetic type. This is no longer
the case, probably because different platforms used to regularly
ignore that requirement.  For example, on cygwin a pthread_t is a
pointer to a structure (a quite common choice), whereas on Linux it
is defined as an 'unsigned long int'.

On cygwin, but not on Linux, 'sparse' currently complains about an
initialiser used on a 'struct load_index_extensions' variable, whose
first field may be a pthread handle (if not compiled with NO_PTHREADS
set).

In order to fix the warning, move the (conditional) pthread field to
the end of the struct and change the initialiser to use a NULL, since
the new (unconditional) first field is a pointer type.

Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
---

Hi Ben,

If you need to re-roll your 'bp/read-cache-parallel' branch, could you
please squash this into the relevant patch (commit a090af334,
"read-cache: load cache extensions on a worker thread", 2018-09-12).

Thanks.

ATB,
Ramsay Jones

 read-cache.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index b27dc01696..6254291742 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1908,13 +1908,13 @@ static void write_eoie_extension(struct strbuf *sb, git_hash_ctx *eoie_context,
 
 struct load_index_extensions
 {
-#ifndef NO_PTHREADS
-	pthread_t pthread;
-#endif
 	struct index_state *istate;
 	const char *mmap;
 	size_t mmap_size;
 	unsigned long src_offset;
+#ifndef NO_PTHREADS
+	pthread_t pthread;
+#endif
 };
 
 static void *load_index_extensions(void *data)
@@ -2145,7 +2145,7 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)
 	const struct cache_header *hdr;
 	const char *mmap;
 	size_t mmap_size;
-	struct load_index_extensions p = { 0 };
+	struct load_index_extensions p = { NULL };
 	unsigned long extension_offset = 0;
 #ifndef NO_PTHREADS
 	int cpus, nr_threads;
-- 
2.19.0

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] read-cache.c: fix a sparse warning
  2018-09-14 23:29 [PATCH] read-cache.c: fix a sparse warning Ramsay Jones
@ 2018-09-16  7:17 ` Eric Sunshine
  2018-09-17 14:15   ` Ben Peart
  0 siblings, 1 reply; 5+ messages in thread
From: Eric Sunshine @ 2018-09-16  7:17 UTC (permalink / raw)
  To: Ramsay Jones; +Cc: Ben Peart, Junio C Hamano, Git List

On Fri, Sep 14, 2018 at 7:29 PM Ramsay Jones
<ramsay@ramsayjones.plus.com> wrote:
> At one time, the POSIX standard required the type used to represent
> a thread handle (pthread_t) be an arithmetic type. This is no longer
> the case, probably because different platforms used to regularly
> ignore that requirement.  For example, on cygwin a pthread_t is a
> pointer to a structure (a quite common choice), whereas on Linux it
> is defined as an 'unsigned long int'.
>
> On cygwin, but not on Linux, 'sparse' currently complains about an
> initialiser used on a 'struct load_index_extensions' variable, whose
> first field may be a pthread handle (if not compiled with NO_PTHREADS
> set).
>
> In order to fix the warning, move the (conditional) pthread field to
> the end of the struct and change the initialiser to use a NULL, since
> the new (unconditional) first field is a pointer type.
>
> Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
> ---
> If you need to re-roll your 'bp/read-cache-parallel' branch, could you
> please squash this into the relevant patch (commit a090af334,
> "read-cache: load cache extensions on a worker thread", 2018-09-12).

The information contained in this commit message is so useful that it
might make sense to plop this patch at the end of the series rather
than merely squashing it in. (Or, if it is squashed, include the above
explanation in the commit message of the appropriate patch.)

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] read-cache.c: fix a sparse warning
  2018-09-16  7:17 ` Eric Sunshine
@ 2018-09-17 14:15   ` Ben Peart
  2018-09-17 16:27     ` Ramsay Jones
  0 siblings, 1 reply; 5+ messages in thread
From: Ben Peart @ 2018-09-17 14:15 UTC (permalink / raw)
  To: Eric Sunshine, Ramsay Jones; +Cc: Ben Peart, Junio C Hamano, Git List



On 9/16/2018 3:17 AM, Eric Sunshine wrote:
> On Fri, Sep 14, 2018 at 7:29 PM Ramsay Jones
> <ramsay@ramsayjones.plus.com> wrote:
>> At one time, the POSIX standard required the type used to represent
>> a thread handle (pthread_t) be an arithmetic type. This is no longer
>> the case, probably because different platforms used to regularly
>> ignore that requirement.  For example, on cygwin a pthread_t is a
>> pointer to a structure (a quite common choice), whereas on Linux it
>> is defined as an 'unsigned long int'.
>>
>> On cygwin, but not on Linux, 'sparse' currently complains about an
>> initialiser used on a 'struct load_index_extensions' variable, whose
>> first field may be a pthread handle (if not compiled with NO_PTHREADS
>> set).
>>
>> In order to fix the warning, move the (conditional) pthread field to
>> the end of the struct and change the initialiser to use a NULL, since
>> the new (unconditional) first field is a pointer type.
>>
>> Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
>> ---
>> If you need to re-roll your 'bp/read-cache-parallel' branch, could you
>> please squash this into the relevant patch (commit a090af334,
>> "read-cache: load cache extensions on a worker thread", 2018-09-12).
> 
> The information contained in this commit message is so useful that it
> might make sense to plop this patch at the end of the series rather
> than merely squashing it in. (Or, if it is squashed, include the above
> explanation in the commit message of the appropriate patch.)
> 

I'm happy to squash it in if I end up re-rolling the patch series.  I'll 
include the information in the commit message above as a comment so that 
it is in close proximity to the code impacted.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] read-cache.c: fix a sparse warning
  2018-09-17 14:15   ` Ben Peart
@ 2018-09-17 16:27     ` Ramsay Jones
  2018-09-17 16:54       ` Ramsay Jones
  0 siblings, 1 reply; 5+ messages in thread
From: Ramsay Jones @ 2018-09-17 16:27 UTC (permalink / raw)
  To: Ben Peart, Eric Sunshine; +Cc: Ben Peart, Junio C Hamano, Git List



On 17/09/18 15:15, Ben Peart wrote:
> 
> 
> On 9/16/2018 3:17 AM, Eric Sunshine wrote:
>> On Fri, Sep 14, 2018 at 7:29 PM Ramsay Jones
>> <ramsay@ramsayjones.plus.com> wrote:
>>> At one time, the POSIX standard required the type used to represent
>>> a thread handle (pthread_t) be an arithmetic type. This is no longer
>>> the case, probably because different platforms used to regularly
>>> ignore that requirement.  For example, on cygwin a pthread_t is a
>>> pointer to a structure (a quite common choice), whereas on Linux it
>>> is defined as an 'unsigned long int'.
>>>
>>> On cygwin, but not on Linux, 'sparse' currently complains about an
>>> initialiser used on a 'struct load_index_extensions' variable, whose
>>> first field may be a pthread handle (if not compiled with NO_PTHREADS
>>> set).
>>>
>>> In order to fix the warning, move the (conditional) pthread field to
>>> the end of the struct and change the initialiser to use a NULL, since
>>> the new (unconditional) first field is a pointer type.
>>>
>>> Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
>>> ---
>>> If you need to re-roll your 'bp/read-cache-parallel' branch, could you
>>> please squash this into the relevant patch (commit a090af334,
>>> "read-cache: load cache extensions on a worker thread", 2018-09-12).
>>
>> The information contained in this commit message is so useful that it
>> might make sense to plop this patch at the end of the series rather
>> than merely squashing it in. (Or, if it is squashed, include the above
>> explanation in the commit message of the appropriate patch.)
>>
> 
> I'm happy to squash it in if I end up re-rolling the patch series.  I'll include the information in the commit message above as a comment so that it is in close proximity to the code impacted.
> 

I will be happy with whatever decision you take regarding whether
to squash this in or add it on top of your series. However, if you
do squash it in, please don't add the commit message info as a
comment to the code. No matter how you word it, I can't imagine
that it would be anything but superfluous - the kind of comment
that would be removed after review! ;-)

The information in the commit message about pthread_t, which I
thought was common knowledge, was not really the main point of
the argument supporting the patch. (Search for "How do I print
a pthread_t", for variations on this theme).

The main point for me: don't conditionally include a field at the
beginning of a structure and then use an initialiser in a variable
declaration. (Unless, I suppose, the first unconditional field had
the same type - but probably not not even then!)

The fact that the conditionally included field itself had an 'opaque'
type was just an additional complication.

ATB,
Ramsay Jones


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] read-cache.c: fix a sparse warning
  2018-09-17 16:27     ` Ramsay Jones
@ 2018-09-17 16:54       ` Ramsay Jones
  0 siblings, 0 replies; 5+ messages in thread
From: Ramsay Jones @ 2018-09-17 16:54 UTC (permalink / raw)
  To: Ben Peart, Eric Sunshine; +Cc: Ben Peart, Junio C Hamano, Git List



On 17/09/18 17:27, Ramsay Jones wrote:
> 
> 
> On 17/09/18 15:15, Ben Peart wrote:
>>
>>
>> On 9/16/2018 3:17 AM, Eric Sunshine wrote:
>>> On Fri, Sep 14, 2018 at 7:29 PM Ramsay Jones
>>> <ramsay@ramsayjones.plus.com> wrote:
>>>> At one time, the POSIX standard required the type used to represent
>>>> a thread handle (pthread_t) be an arithmetic type. This is no longer
>>>> the case, probably because different platforms used to regularly
>>>> ignore that requirement.  For example, on cygwin a pthread_t is a
>>>> pointer to a structure (a quite common choice), whereas on Linux it
>>>> is defined as an 'unsigned long int'.
>>>>
>>>> On cygwin, but not on Linux, 'sparse' currently complains about an
>>>> initialiser used on a 'struct load_index_extensions' variable, whose
>>>> first field may be a pthread handle (if not compiled with NO_PTHREADS
>>>> set).
>>>>
>>>> In order to fix the warning, move the (conditional) pthread field to
>>>> the end of the struct and change the initialiser to use a NULL, since
>>>> the new (unconditional) first field is a pointer type.
>>>>
>>>> Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
>>>> ---
>>>> If you need to re-roll your 'bp/read-cache-parallel' branch, could you
>>>> please squash this into the relevant patch (commit a090af334,
>>>> "read-cache: load cache extensions on a worker thread", 2018-09-12).
>>>
>>> The information contained in this commit message is so useful that it
>>> might make sense to plop this patch at the end of the series rather
>>> than merely squashing it in. (Or, if it is squashed, include the above
>>> explanation in the commit message of the appropriate patch.)
>>>
>>
>> I'm happy to squash it in if I end up re-rolling the patch series.  I'll include the information in the commit message above as a comment so that it is in close proximity to the code impacted.
>>
> 
> I will be happy with whatever decision you take regarding whether
> to squash this in or add it on top of your series. However, if you
> do squash it in, please don't add the commit message info as a
> comment to the code. No matter how you word it, I can't imagine
> that it would be anything but superfluous - the kind of comment
> that would be removed after review! ;-)
> 
> The information in the commit message about pthread_t, which I
> thought was common knowledge, was not really the main point of
> the argument supporting the patch. (Search for "How do I print
> a pthread_t", for variations on this theme).
> 
> The main point for me: don't conditionally include a field at the
> beginning of a structure and then use an initialiser in a variable
> declaration. (Unless, I suppose, the first unconditional field had
> the same type - but probably not not even then!)
> 
> The fact that the conditionally included field itself had an 'opaque'
> type was just an additional complication.

BTW, I just noticed that you explicitly initialise each field of
that structure (not surprising), so an even simpler solution is
to simply remove the unneeded initialiser! ;-)

ATB,
Ramsay Jones


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-14 23:29 [PATCH] read-cache.c: fix a sparse warning Ramsay Jones
2018-09-16  7:17 ` Eric Sunshine
2018-09-17 14:15   ` Ben Peart
2018-09-17 16:27     ` Ramsay Jones
2018-09-17 16:54       ` Ramsay Jones

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox