Re: [PATCH 7/7] Accelerate rename_dst setup

Elijah Newren [off-list ref] writes:

It's identical for runtime correctness, yes.  But the primary point
isn't what happens at runtime, but what happens when future code
readers come along.  If I only keep the "if (dst->is_rename)" line
that comes after without the assert, then someone in the future will
come along (maybe even myself) and think "the original author wasn't
being careful here; they should change this to a check on (dst &&
dst->is_rename)" (because honestly, it is the kind of mistake I'm
prone to make).  However, if they were to do so, and some bug gets
introduced so that locate_rename_dst() returns a NULL for a pair of
interest, then they've masked a bug in the algorithm and made it fail
in harder-to-detect-and-track-down ways.  I wanted it to be clear that
dst == NULL is unacceptable.  I guess I could have marked it with
BUG(), but between an assertion and a NULL-pointer indirection, I
figured that code aborting under that condition was pretty well
covered.  :-)

assert() often gets turned into no-op, so it is more like leaving a
comment in the code.  A comment or BUG("text") can describe not just
the fact that reaching this point with dst==NULL is a bug in the
caller but also why it is a bug (e.g. how NULL-dst would screw up
the computation downstream from this point), but an assert() cannot.