From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS31976 209.132.180.0/23 X-Spam-Status: No, score=-3.7 required=3.0 tests=AWL,BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,RCVD_IN_DNSWL_HI shortcircuit=no autolearn=ham autolearn_force=no version=3.4.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by dcvr.yhbt.net (Postfix) with ESMTP id 13DB51F404 for ; Mon, 17 Sep 2018 16:27:29 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728455AbeIQVzc (ORCPT ); Mon, 17 Sep 2018 17:55:32 -0400 Received: from avasout07.plus.net ([84.93.230.235]:37650 "EHLO avasout07.plus.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728016AbeIQVzc (ORCPT ); Mon, 17 Sep 2018 17:55:32 -0400 Received: from [10.0.2.15] ([80.189.70.183]) by smtp with ESMTPA id 1wMugFJeFjlDz1wMwg1zP3; Mon, 17 Sep 2018 17:27:26 +0100 X-CM-Score: 0.00 X-CNFS-Analysis: v=2.3 cv=GrdsBH9C c=1 sm=1 tr=0 a=6SF67mWK+VR8hB1Kjo6y2g==:117 a=6SF67mWK+VR8hB1Kjo6y2g==:17 a=IkcTkHD0fZMA:10 a=EBOSESyhAAAA:8 a=zjR36oVeXIAJ1s83-LAA:9 a=QEXdDO2ut3YA:10 a=yJM6EZoI5SlJf8ks9Ge_:22 X-AUTH: ramsayjones@:2500 Subject: Re: [PATCH] read-cache.c: fix a sparse warning To: Ben Peart , Eric Sunshine Cc: Ben Peart , Junio C Hamano , Git List References: <2efc8b66-3d26-f794-b5fe-6a4b013feeaf@gmail.com> From: Ramsay Jones Message-ID: Date: Mon, 17 Sep 2018 17:27:24 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <2efc8b66-3d26-f794-b5fe-6a4b013feeaf@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-GB Content-Transfer-Encoding: 8bit X-CMAE-Envelope: MS4wfBbQIncu5VUmesXcSH5ErfskPKJiR/XdtwgEcjrVrlOcD7S1bbp+NkPv0JDrRMiJef8jHU9axjGtvY9uHru91vOqTqLA+23l7t5dl50zbu+90dVKV5A8 hqnJeePFYP3R0bexv7+syAAmEOKv5thZOzZ8Ruir4tYu8V85oE0mgqv+nuAGLX6DuFH0+jp79TxG2g== Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org 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 >> 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 >>> --- >>> 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