On Mon, 10 Aug 2020 at 23:27, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
>
> On Mon, Aug 10, 2020 at 3:48 PM Martin Ågren <martin.agren@xxxxxxxxx> wrote:
> > void stop_progress(struct progress **p_progress)
> > {
> > + if (!p_progress)
> > + BUG("don't provide NULL to stop_progress");
> > +
> > finish_if_sparse(*p_progress);
>
> I'm wondering what this really buys us over simply crashing due to the
> NULL dereference (aside from the slightly more informative diagnostic
> message). Either way, it's going to crash, as it should because
> passing NULL is indeed a programmer error for these two functions. I'm
> pretty sure that it is more common in this project simply to allow a
> programmer error like this simply to crash on its own rather than
> adding code to make it crash explicitly.
I'm not a big fan of undefined behavior. In general, I don't buy the
"but in practice it will do what we want" argumentation.
Before 98a1364740 ("trace2: log progress time and throughput",
2020-05-12), we didn't check for NULL in this function. Then that commit
tried to do so. It would feel wrong for me to say "that commit didn't
get it quite right -- rip out the check". Then, to be honest, I'd much
rather just leave it in place. At least that way, someone else might
spot it a year from now.
I could add an early return (instead of an early BUG). That would
gracefully handle NULL. Grepping around suggests there are other `if (!p)
BUG();`. Even Documentation/CodingGuidelines BUGs on a NULL-pointer,
although in the context of checking for NULL (as opposed to "how to
BUG").
> > - if (p_progress && *p_progress) {
> > + if (*p_progress) {
>
> In other words, I think the entire patch can be reduced to just this
> change here (and a simplified commit message).
I started with this, but then I felt terrible for just sweeping the
whole thing under the rug.
Martin
