[PATCH] D63859: [Support] Add getFileOrSTDIN which calls a callback if stdin is a tty

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/Support/MemoryBuffer.h:130
 
+  /// If Filename is "-" and stdin refers to a terminal, ie input has not been
+  /// redirected in, call TtyStdin then exit with code 1. Otherwise acts the
----------------
ie -> i.e.


================
Comment at: llvm/include/llvm/Support/MemoryBuffer.h:134
+  static ErrorOr<std::unique_ptr<MemoryBuffer>>
+  getFileOrSTDIN(const Twine &Filename, std::function<void(void)> TtyStdin,
+                 int64_t FileSize = -1, bool RequiresNullTerminator = true);
----------------
Do you actually need an overload? Can this just be added as a default parameter for the other getFileOrSTDIN?


================
Comment at: llvm/lib/Support/MemoryBuffer.cpp:164
+      TtyStdin();
+      std::exit(1);
+    }
----------------
This shoudn't be calling std::exit. Either it should always be an error if the code gets here (in which case TtyStdin return an error code), or you need to have TtyStdin return a valid MemoryBuffer. Either way, the lambda signature is wrong.

If TtyStdin is intended to handle errors, then it needs renaming to clearly indicate this. Perhaps giving it a name indicating when it is used would make it clearer.


================
Comment at: llvm/unittests/Support/MemoryBufferTest.cpp:19
 #include "gtest/gtest.h"
+#include <unistd.h> // for dup2(2)
 
----------------
Have you tried this on Windows. I suspect you'll find it won't work as is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63859