On Tue, Dec 28 2021, René Scharfe wrote:
> Am 28.12.21 um 16:19 schrieb Ævar Arnfjörð Bjarmason:
>> We have over 50 uses of "isatty(1)" and "isatty(2)" in the codebase,
>> and around 10 "isatty(0)", but three callers used the
>> {STDIN_FILENO,STD{OUT,ERR}_FILENO} macros in "stdlib.h" to refer to
>> them.
>>
>> Let's change these for consistency. This makes it easier to change
>> all calls to isatty() at a whim, which is useful to test some
>> scenarios[1].
>
> Hmm. Matching e.g. "(0|STDIN_FILENO)" instead of "0" is harder, of
> course, but not much.
>
> Shouldn't we use these macros more to reduce the number of magic values?
> The code is slightly easier to read before this patch because it doesn't
> require the reader to know the meaning of these numbers.
>
> Reducing the constants to their numerical values is easy to automate in
> general; the opposite direction is harder. Coccinelle can help us take
> such a step with a semantic patch like this:
>
> @@
> @@
> isatty(
> (
> - 0
> + STDIN_FILENO
> |
> - 1
> + STDOUT_FILENO
> |
> - 2
> + STDERR_FILENO
> )
> )
We don't bother with EXIT_SUCCESS and EXIT_FAILURE, and for those (VMS)
there is a reason to not use the constants, as EXIT_FAILURE may differ.
But for these I personally think these symbolic names are rather
useless.
They never differ, and when working on POSIX systems you're going to
need to know that 1 is stdout, 2 is stderr. You're also going to have to
maintain shellscripts that use ">&2" or whatever. Those aren't using a
hypothetical ">&$STDERR_FILENO".
But in any case, this change isn't even trying to make the argument that
we *should* use one over the other, just that the constants are used
much more than *_FILENO, so changing them to make a subsequent (now
ejected out of this series) change easier to explain is worth it.
So I'd think we can just take this small change, and argue separately
whether it's worth it to apply that coccinelle rule.
