Am 09.03.2017 um 12:24 schrieb Johannes Schindelin:
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é