Re: [PATCH v3 22/22] builtin/diff: free symmetric diff members

[Junio C Hamano <gitster@pobox.com>]

Patrick Steinhardt <ps@pks.im> writes:

> Good point. It does make sense for `_free()` functions to handle NULL
> pointers, but doesn't quite for `_release()` ones.

I agree that foo_free() should accept NULL and silently become a
no-op.  I do not care deeply whether foo_release() did the same, or
not, as long as all *_release()s behave the same way.  Maybe it is
more convenient if they ignored NULL, as I have a hunch that feeding
a NULL pointer to foo_release() is unlikely to be a bug.

Since we documented our aspiration to use these (and foo_clear())
consistently, we may #leftoverbits want to also document the calling
convention as well.

>> And symdiff_prepare() at least clears its .skip member to NULL, so
>> this pre-initialization is probably not needed.  If we are preparing
>> ourselves for future changes of the flow in this function (e.g.
>> goto's that jump to the clean-up label from which symdiff_release()
>> is always called, even when we did not call symdiff_prepare() on
>> this thing), this is probably not sufficient to convey that
>> intention (instead I'd use an explicit ".skip = NULL" to say "we
>> might not even call _prepare() but this one is prepared to be passed
>> to _release() even in such a case").
>> 
>> Given that there is no such goto exists, and that _prepare() always
>> sets up the .skip member appropriately, I wonder if we are much
>> better off leaving sdiff uninitialized at the declaration site here.
>> If we add such a goto that bypasses _prepare() in the future, the
>> compiler will notice that we are passing an uninitialized sdiff to
>> _release(), no?
>
> You'd hope it does, but it certainly depends on your compiler flags.
> Various hardening flags for example implicitly initialize variables, and
> I have a feeling that this also causes them to not emit any warnings
> anymore. At least I only spot such warnings in CI.

Yeah, that is a sad fact in the real world X-<.  To be defensive, I
think an explicit "{ .skip = NULL }" or "{ 0 }" would not be too bad
and may even serve as a good reminder for developers who may want to
jump over the call to _prepare() in the future.

The explicit ".skip = NULL" says "we know it is safe to call
_release() with a struct that hasn't gone through _prepare(), as
long as its .skip member is cleared", but the story "{ 0 }" tells us
is not much more than "we clear just like everybody else", and that
is why I suggested the former (iow, I know both mean the same thing
to the C compiler---I just care more about what it tells the human
readers).

Thanks.