{"thread":{"id":"49351","subject":"[PATCH] read-cache.c: fix a sparse warning","startedAt":"2018-09-14T23:29:26Z","lastAt":"2018-09-17T16:54:57Z","messageCount":5,"participants":["Ramsay Jones","Eric Sunshine","Ben Peart"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"358171","messageId":"c92b2f25-315d-141b-8974-dd05d7fd2b9e@ramsayjones.plus.com","threadId":"49351","inReplyTo":null,"subject":"[PATCH] read-cache.c: fix a sparse warning","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2018-09-14T23:29:22Z","receivedAt":"2018-09-14T23:29:26Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\nAt one time, the POSIX standard required the type used to represent\na thread handle (pthread_t) be an arithmetic type. This is no longer\nthe case, probably because different platforms used to regularly\nignore that requirement. For example, on cygwin a pthread_t is a\npointer to a structure (a quite common choice), whereas on Linux it\nis defined as an 'unsigned long int'.\n\nOn cygwin, but not on Linux, 'sparse' currently complains about an\ninitialiser used on a 'struct load_index_extensions' variable, whose\nfirst field may be a pthread handle (if not compiled with NO_PTHREADS\nset).\n\nIn order to fix the warning, move the (conditional) pthread field to\nthe end of the struct and change the initialiser to use a NULL, since\nthe new (unconditional) first field is a pointer type.\n\nSigned-off-by: Ramsay Jones \n---\n\nHi Ben,\n\nIf you need to re-roll your 'bp/read-cache-parallel' branch, could you\nplease squash this into the relevant patch (commit a090af334,\n\"read-cache: load cache extensions on a worker thread\", 2018-09-12).\n\nThanks.\n\nATB,\nRamsay Jones\n\n read-cache.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex b27dc01696..6254291742 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1908,13 +1908,13 @@ static void write_eoie_extension(struct strbuf *sb, git_hash_ctx *eoie_context,\n \n struct load_index_extensions\n {\n-#ifndef NO_PTHREADS\n-\tpthread_t pthread;\n-#endif\n \tstruct index_state *istate;\n \tconst char *mmap;\n \tsize_t mmap_size;\n \tunsigned long src_offset;\n+#ifndef NO_PTHREADS\n+\tpthread_t pthread;\n+#endif\n };\n \n static void *load_index_extensions(void *data)\n@@ -2145,7 +2145,7 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)\n \tconst struct cache_header *hdr;\n \tconst char *mmap;\n \tsize_t mmap_size;\n-\tstruct load_index_extensions p = { 0 };\n+\tstruct load_index_extensions p = { NULL };\n \tunsigned long extension_offset = 0;\n #ifndef NO_PTHREADS\n \tint cpus, nr_threads;\n-- \n2.19.0\n"},{"id":"358225","messageId":"CAPig+cSm52i=ypGoPGGumhP+_waq=OU2QZ=p3oVzTTzLsNpfNA@mail.gmail.com","threadId":"49351","inReplyTo":"c92b2f25-315d-141b-8974-dd05d7fd2b9e@ramsayjones.plus.com","subject":"Re: [PATCH] read-cache.c: fix a sparse warning","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-09-16T07:17:29Z","receivedAt":"2018-09-16T07:17:45Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Sep 14, 2018 at 7:29 PM Ramsay Jones\n wrote:\n> At one time, the POSIX standard required the type used to represent\n> a thread handle (pthread_t) be an arithmetic type. This is no longer\n> the case, probably because different platforms used to regularly\n> ignore that requirement. For example, on cygwin a pthread_t is a\n> pointer to a structure (a quite common choice), whereas on Linux it\n> is defined as an 'unsigned long int'.\n>\n> On cygwin, but not on Linux, 'sparse' currently complains about an\n> initialiser used on a 'struct load_index_extensions' variable, whose\n> first field may be a pthread handle (if not compiled with NO_PTHREADS\n> set).\n>\n> In order to fix the warning, move the (conditional) pthread field to\n> the end of the struct and change the initialiser to use a NULL, since\n> the new (unconditional) first field is a pointer type.\n>\n> Signed-off-by: Ramsay Jones \n> ---\n> If you need to re-roll your 'bp/read-cache-parallel' branch, could you\n> please squash this into the relevant patch (commit a090af334,\n> \"read-cache: load cache extensions on a worker thread\", 2018-09-12).\n\nThe information contained in this commit message is so useful that it\nmight make sense to plop this patch at the end of the series rather\nthan merely squashing it in. (Or, if it is squashed, include the above\nexplanation in the commit message of the appropriate patch.)\n"},{"id":"358270","messageId":"2efc8b66-3d26-f794-b5fe-6a4b013feeaf@gmail.com","threadId":"49351","inReplyTo":"CAPig+cSm52i=ypGoPGGumhP+_waq=OU2QZ=p3oVzTTzLsNpfNA@mail.gmail.com","subject":"Re: [PATCH] read-cache.c: fix a sparse warning","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2018-09-17T14:15:39Z","receivedAt":"2018-09-17T14:15:44Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 9/16/2018 3:17 AM, Eric Sunshine wrote:\n> On Fri, Sep 14, 2018 at 7:29 PM Ramsay Jones\n> wrote:\n>> At one time, the POSIX standard required the type used to represent\n>> a thread handle (pthread_t) be an arithmetic type. This is no longer\n>> the case, probably because different platforms used to regularly\n>> ignore that requirement. For example, on cygwin a pthread_t is a\n>> pointer to a structure (a quite common choice), whereas on Linux it\n>> is defined as an 'unsigned long int'.\n>>\n>> On cygwin, but not on Linux, 'sparse' currently complains about an\n>> initialiser used on a 'struct load_index_extensions' variable, whose\n>> first field may be a pthread handle (if not compiled with NO_PTHREADS\n>> set).\n>>\n>> In order to fix the warning, move the (conditional) pthread field to\n>> the end of the struct and change the initialiser to use a NULL, since\n>> the new (unconditional) first field is a pointer type.\n>>\n>> Signed-off-by: Ramsay Jones \n>> ---\n>> If you need to re-roll your 'bp/read-cache-parallel' branch, could you\n>> please squash this into the relevant patch (commit a090af334,\n>> \"read-cache: load cache extensions on a worker thread\", 2018-09-12).\n> \n> The information contained in this commit message is so useful that it\n> might make sense to plop this patch at the end of the series rather\n> than merely squashing it in. (Or, if it is squashed, include the above\n> explanation in the commit message of the appropriate patch.)\n> \n\nI'm happy to squash it in if I end up re-rolling the patch series. I'll \ninclude the information in the commit message above as a comment so that \nit is in close proximity to the code impacted.\n"},{"id":"358286","messageId":"d7c66568-0e6d-9d92-4559-d4963c0412ec@ramsayjones.plus.com","threadId":"49351","inReplyTo":"2efc8b66-3d26-f794-b5fe-6a4b013feeaf@gmail.com","subject":"Re: [PATCH] read-cache.c: fix a sparse warning","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2018-09-17T16:27:24Z","receivedAt":"2018-09-17T16:27:29Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 17/09/18 15:15, Ben Peart wrote:\n> \n> \n> On 9/16/2018 3:17 AM, Eric Sunshine wrote:\n>> On Fri, Sep 14, 2018 at 7:29 PM Ramsay Jones\n>> wrote:\n>>> At one time, the POSIX standard required the type used to represent\n>>> a thread handle (pthread_t) be an arithmetic type. This is no longer\n>>> the case, probably because different platforms used to regularly\n>>> ignore that requirement.  For example, on cygwin a pthread_t is a\n>>> pointer to a structure (a quite common choice), whereas on Linux it\n>>> is defined as an 'unsigned long int'.\n>>>\n>>> On cygwin, but not on Linux, 'sparse' currently complains about an\n>>> initialiser used on a 'struct load_index_extensions' variable, whose\n>>> first field may be a pthread handle (if not compiled with NO_PTHREADS\n>>> set).\n>>>\n>>> In order to fix the warning, move the (conditional) pthread field to\n>>> the end of the struct and change the initialiser to use a NULL, since\n>>> the new (unconditional) first field is a pointer type.\n>>>\n>>> Signed-off-by: Ramsay Jones \n>>> ---\n>>> If you need to re-roll your 'bp/read-cache-parallel' branch, could you\n>>> please squash this into the relevant patch (commit a090af334,\n>>> \"read-cache: load cache extensions on a worker thread\", 2018-09-12).\n>>\n>> The information contained in this commit message is so useful that it\n>> might make sense to plop this patch at the end of the series rather\n>> than merely squashing it in. (Or, if it is squashed, include the above\n>> explanation in the commit message of the appropriate patch.)\n>>\n> \n> 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.\n> \n\nI will be happy with whatever decision you take regarding whether\nto squash this in or add it on top of your series. However, if you\ndo squash it in, please don't add the commit message info as a\ncomment to the code. No matter how you word it, I can't imagine\nthat it would be anything but superfluous - the kind of comment\nthat would be removed after review! ;-)\n\nThe information in the commit message about pthread_t, which I\nthought was common knowledge, was not really the main point of\nthe argument supporting the patch. (Search for \"How do I print\na pthread_t\", for variations on this theme).\n\nThe main point for me: don't conditionally include a field at the\nbeginning of a structure and then use an initialiser in a variable\ndeclaration. (Unless, I suppose, the first unconditional field had\nthe same type - but probably not not even then!)\n\nThe fact that the conditionally included field itself had an 'opaque'\ntype was just an additional complication.\n\nATB,\nRamsay Jones\n\n"},{"id":"358294","messageId":"ae1b3014-f56a-d379-2d76-03f95a4cf388@ramsayjones.plus.com","threadId":"49351","inReplyTo":"d7c66568-0e6d-9d92-4559-d4963c0412ec@ramsayjones.plus.com","subject":"Re: [PATCH] read-cache.c: fix a sparse warning","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2018-09-17T16:54:53Z","receivedAt":"2018-09-17T16:54:57Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 17/09/18 17:27, Ramsay Jones wrote:\n> \n> \n> On 17/09/18 15:15, Ben Peart wrote:\n>>\n>>\n>> On 9/16/2018 3:17 AM, Eric Sunshine wrote:\n>>> On Fri, Sep 14, 2018 at 7:29 PM Ramsay Jones\n>>> wrote:\n>>>> At one time, the POSIX standard required the type used to represent\n>>>> a thread handle (pthread_t) be an arithmetic type. This is no longer\n>>>> the case, probably because different platforms used to regularly\n>>>> ignore that requirement.  For example, on cygwin a pthread_t is a\n>>>> pointer to a structure (a quite common choice), whereas on Linux it\n>>>> is defined as an 'unsigned long int'.\n>>>>\n>>>> On cygwin, but not on Linux, 'sparse' currently complains about an\n>>>> initialiser used on a 'struct load_index_extensions' variable, whose\n>>>> first field may be a pthread handle (if not compiled with NO_PTHREADS\n>>>> set).\n>>>>\n>>>> In order to fix the warning, move the (conditional) pthread field to\n>>>> the end of the struct and change the initialiser to use a NULL, since\n>>>> the new (unconditional) first field is a pointer type.\n>>>>\n>>>> Signed-off-by: Ramsay Jones \n>>>> ---\n>>>> If you need to re-roll your 'bp/read-cache-parallel' branch, could you\n>>>> please squash this into the relevant patch (commit a090af334,\n>>>> \"read-cache: load cache extensions on a worker thread\", 2018-09-12).\n>>>\n>>> The information contained in this commit message is so useful that it\n>>> might make sense to plop this patch at the end of the series rather\n>>> than merely squashing it in. (Or, if it is squashed, include the above\n>>> explanation in the commit message of the appropriate patch.)\n>>>\n>>\n>> 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.\n>>\n> \n> I will be happy with whatever decision you take regarding whether\n> to squash this in or add it on top of your series. However, if you\n> do squash it in, please don't add the commit message info as a\n> comment to the code. No matter how you word it, I can't imagine\n> that it would be anything but superfluous - the kind of comment\n> that would be removed after review! ;-)\n> \n> The information in the commit message about pthread_t, which I\n> thought was common knowledge, was not really the main point of\n> the argument supporting the patch. (Search for \"How do I print\n> a pthread_t\", for variations on this theme).\n> \n> The main point for me: don't conditionally include a field at the\n> beginning of a structure and then use an initialiser in a variable\n> declaration. (Unless, I suppose, the first unconditional field had\n> the same type - but probably not not even then!)\n> \n> The fact that the conditionally included field itself had an 'opaque'\n> type was just an additional complication.\n\nBTW, I just noticed that you explicitly initialise each field of\nthat structure (not surprising), so an even simpler solution is\nto simply remove the unneeded initialiser! ;-)\n\nATB,\nRamsay Jones\n\n"}]}