Re: [PATCH v8 7/7] *.c: use isatty(0|2), not isatty(STDIN_FILENO|STDERR_FILENO)

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Dec 28, 2021, 23:56 UTC
Message-ID: <211229.86zgokgttz.gmgdl@evledraar.gmail.com>
In-Reply-To: <d0bdc7e5-9e34-49d8-33cf-dd96a807617e@web.de>

On Tue, Dec 28 2021, René Scharfe wrote:

Show 35 quoted lines

> 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.