[PATCH] D63858: [Support] Add sys::fs::is_tty() wrapper around isatty(3)

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Code change looks good, but one very weird thing I noticed is that your change in the header is in "FileSystem.h" but the definitions are in "Path.inc". This seems wrong, and should probably be fixed, possibly by adding FileSystem.inc?

Also, you should definitely get some more reviewers to look at this, as I don't want to be making the decision on that alone. Take a look at the commits history for the files you touched.



================
Comment at: llvm/include/llvm/Support/FileSystem.h:640
+/// @param FD Input file descriptor.
+/// @returns true if the file descriptor refers to a terminal.
+bool is_tty(int FD);
----------------
What about a) if the file descriptor is invalid (doesn't refer to anything) or b) doesn't refer to a terminal (in other words, be explicit with the false as well as the true)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63858