Re: [PATCH v16 Part II 2/8] bisect--helper: `bisect_write` shell function in C
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Nov 23, 2018, 10:13 UTC
- Message-ID
- <nycvar.QRO.7.76.6.1811231111030.41@tvgsbejvaqbjf.bet>
- In-Reply-To
- <CAFZEwPP7dkWwRJD2ohDfnV_Phb0ga7YPZoVC920JPrQXLAGekw@mail.gmail.com>
Hi Pranit,
(Cc:ing Tanushree because they will try to pick up this patch series as part of the Outreachy program.)
On Mon, 30 Oct 2017, Pranit Bauva wrote:
Show 25 quoted lines
> On Fri, Oct 27, 2017 at 10:58 PM, Martin Ågren <martin.agren@gmail.com> wrote:
> > On 27 October 2017 at 17:06, Pranit Bauva <pranit.bauva@gmail.com> wrote:
> >> +static void free_terms(struct bisect_terms *terms)
> >> +{
> >> + if (!terms->term_good)
> >> + free((void *) terms->term_good);
> >> + if (!terms->term_bad)
> >> + free((void *) terms->term_bad);
> >> +}
> >
> > These look like no-ops. Remove `!` for correctness, or `if (...)` for
> > simplicity, since `free()` can handle NULL.
>
> I probably forgot to do this here. I will make the change.
>
> > You leave the pointers dangling, but you're ok for now since this is the
> > last thing that happens in `cmd_bisect__helper()`. Your later patches
> > add more users, but they're also ok, since they immediately assign new
> > values.
> >
> > In case you (and others) find it useful, the below is a patch I've been
> > sitting on for a while as part of a series to plug various memory-leaks.
> > `FREE_AND_NULL_CONST()` would be useful in precisely these situations.
>
> Honestly, I wouldn't be the best person to judge this.Git's source code implicitly assumes that any `const` pointer refers to memory owned by another code path. It is therefore probably not a good idea to encourage `free()`ing a `const` pointer.
Which brings me back to the question: who really owns that allocated memory to which `term_good` and `term_bad` refer?
Ciao, Johannes