Re: [RFC PATCH 03/15] reftable: don't memset() a NULL from failed malloc()

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jun 6, 2022, 17:38 UTC
Message-ID: <220606.864k0xwuyl.gmgdl@evledraar.gmail.com>
In-Reply-To: <xmqq7d5tag3s.fsf@gitster.g>

On Mon, Jun 06 2022, Junio C Hamano wrote:

Show 13 quoted lines

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> [...]
>> The question shouldn't be whether those things in particular were worth
>> the effort, but whether the added safety of getting the new diagnostic
>> going forward is worth the one-time cost.
>
> Workarounds for false positives are hardly one-time cost.  They have
> to stay with us until the -fanalyzer gets corrected, somebody needs
> to remember to occasionally check if that happened, and revert the
> workaround to bring the code into their more natural form.  What has
> been good and readable for human programmers for a long time does
> not need to be touched just to work around a false positive bug in a
> new tool.

Yes, but I think in this case most of these are either 100% legitimate issues, or things where we'd like to e.g. add a BUG() assertion anyway, e.g. in the diff parsing case.

Show 8 quoted lines

>> I find the warning output from -fanalyzer to be *really useful*.
>
> I do not mind if you add -fanalyzer during your build via your own
> config.mak file, and you would help them improve the analyzer by
> reporting false positive bugs while finding possible bugs in the
> code we have (like you did in a few patches in this series) and the
> code you are writing.  You are capable enough to keep your own set
> of patches to work around their false positive bugs locally.

Of course.

> But if you have to send in 15 patches with more workaround changes
> than real fix, then it is premature for us to bear the cost to have
> workaround for the tool.

The idea here was to send an RFC showing what it would take to get it into a state where it would be more useful to others.

I.e. I've found it useful to run with it in my own builds and see if anything crops up that's not on the whitelist, I was aiming to give that to others with an easily tweakable knob.

> There are folks who use our codebase as a suitably-sized guinea pig
> to improve their tool, and we should not make it harder for them to
> do so, but our priority is to improve the product of this project.

Those people can still use CFLAGS=-fanalyzer

> Come to think of it, adding unnecessary workarounds is a hostile act
> to those who are trying to improve -fanalyzer, I guess, too.  They
> may want to fix problems in their tool, but workarounds hide them.

I'm not proposing that we do anything we wouldn't otherwise do to appease -fanalyzer, but that we:

 1. Fix things it alerts on because we'd find it worthwhile anyway
 2. For the rest, have a macro to appease it.

That macro being similar to e.g. UNLEAK() in that we'd opt-in enable it if you enabled certain flags, but otherwise we'd ignore it.