Subject: Re: [PATCH] trace2: refactor to avoid gcc warning under -O3

From: Junio C Hamano <[email-masked]>
Date: Thu, 06 May 2021 10:26:34 +0900

Johannes Schindelin <[email-masked]> writes:

> Hi Ævar,
>
> On Wed, 5 May 2021, Ævar Arnfjörð Bjarmason wrote:
>
>> Refactor tr2_dst_try_uds_connect() to avoid a gcc warning[1] that
>> appears under -O3 (but not -O2). This makes the build pass under
>> DEVELOPER=1 without needing a DEVOPTS=no-error.
>>
>> This can be reproduced with GCC Debian 8.3.0-6, but not e.g. with
>> clang 7.0.1-8+deb10u2. We've had this warning since
>> ee4512ed481 (trace2: create new combined trace facility, 2019-02-22).
>>
>> As noted in [2] this warning happens because the compiler doesn't
>> assume that errno must be non-zero after a failed syscall. Let's work
>> around it as suggested in that analysis. We now return -1 ourselves on
>> error, and save away the value of errno in a variable the caller
>> passes in.
>
> It would probably be a lot nicer if you lead with this insight. I could
> imagine, for example, that a oneline like this would be much more helpful
> to any reader:
>
> 	trace2: do not assume errno != 0 after a failed syscall

But that is misleading.  

My understanding is that this patch is about working around
compilers that do not know that a failed syscall means errno would
be set to non-zero.  Am I mistaken?

Otherwise I'd strongly prefer to see a word that hints that this is
an otherwise unneeded workaround for comiplers.  Your suggested
title instead hints that it is wrong to assume that errno will be
set to non-zero after a syscall.  I do not think that is the message
we want to send to our readers.