[PATCH] D75278: Use FileDescriptorIsDisplayed in place of direct call to isatty in support library

[Douglas Gliner via Phabricator via llvm-commits]

dgg5503 added a comment.

Hi @MaskRay,

> At the least, the target OS should be mentioned.

This change is required for the PlayStation 4 operating system. I suppose the OS would be considered exotic so it's understandable if this change isn't desired. However, when I saw `isatty` being directly called in this function, it wasn't immediately obvious to me that the preprocessor block guaranteed that section of code to be POSIX compliant. This was also the only other spot in the LLVM folder that called `isatty` directly except for some libraries under `llvm/utils` and the function `FileDescriptorIsDisplayed` under the Unix platform for the Support lib. I figured this was simply an oversight especially since in `FileDescriptorIsDisplayed`, `HAVE_ISATTY` guards the use of `isatty` when not available on the target Unix-like system.

> A FreeBSD dev said that it's a "fix your libc" situation.
>  If unistd.h does not provide isatty, it will be difficult to imagine how many other things will break. Supporting an exotic OS will lay the burden on developers who maintain the file.

I see in D75707 <https://reviews.llvm.org/D75707> that you are removing `HAVE_ISATTY`, so I guess it was a mistake to have the guard in the Unix support library? I'd be ok waiting for the result of D75707 <https://reviews.llvm.org/D75707> before continuing discussion here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75278/new/

https://reviews.llvm.org/D75278