Re: [PATCH 2/2] Fix callsites of real_pathdup() that wanted it to die on error

From: René Scharfe <l.s.r@web.de>
Date: Mar 9, 2017, 16:33 UTC
Message-ID: <40b64644-f01c-d748-1cc3-8b3046be8a83@web.de>
In-Reply-To: <alpine.DEB.2.20.1703091221440.3767@virtualbox>

Am 09.03.2017 um 12:24 schrieb Johannes Schindelin:

Show 7 quoted lines

> While I would have agreed earlier that René's patch looks less intrusive,
> I have to point out that there would not have been any possible regression
> if the original patch had introduced the die_on_error parameter. It would
> have made the contract *obvious*.
>
> The nicer API made the contract unobvious, and that was the reason that
> the bug could hide.

You mean nicer in the sense of more forgiving, right?

The meaning of calls of a function with a die_on_error parameter are only obvious if at least the name of that parameter is known. A declaration without it would not suffice:

	char *real_pathdup(const char *, int);

There are two possibilities that can't be distinguished by looking at callers alone:

	char *real_pathdup(const char *path, int die_on_error);
	char *real_pathdup(const char *path, int gentle);

An enum can help, so that calls look something like this:

	copy_or_death = real_pathdup(path, DIE_ON_ERROR);
	copy = real_pathdup(path, IGNORE_ERRORS); if (copy) ...

For binary flags we can easily encode that information into the function names and thus simplify the API:

	copy_or_death = real_pathdup_or_die(path);
	copy = real_pathdup_gently(path); if (copy) ...

And we can drop the suffix of the variant used overwhelmingly often, but as you pointed out that relies on readers knowing that convention.

I'm sure Brandon will balance these concerns nicely in follow-up patches. ;)

René