Hi Junio, On Thu, 6 May 2021, Junio C Hamano wrote: > Johannes Schindelin <Johannes.Schindelin@xxxxxx> 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. Right, the oneline I suggested was only for the original patch, with which I disagreed. Ciao, Dscho
