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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I've made more comments, but in researching `dup2` options, I stumbled on `StandardInIsUserInput` which might make this whole function redundant. Does it satisfy your use-case?



================
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);
----------------
abrachet wrote:
> jhenderson wrote:
> > Do you actually need an overload? Can this just be added as a default parameter for the other getFileOrSTDIN?
> I think so. The alternative is to put it before FileSize, which isn't great because a caller might actually be specifying this somewhere, and at the end is worse if they need to specify FileSize and RequiresNullTerminator.
But can't the people who want to call this new overload just explicitly specify the FileSize and RequiresNullTerminator fields, and then provide the callback as the final argument? Other call sites would be completely unaffected.


================
Comment at: llvm/include/llvm/Support/MemoryBuffer.h:131
+  /// If Filename is "-" and stdin refers to a terminal, i.e. input has not been
+  /// redirected in, call NotRedirectedInput and return its MemoryBuffer.
+  /// Otherwise acts the same as
----------------
"return its result." (It could be a nullptr for example).


================
Comment at: llvm/lib/Support/MemoryBuffer.cpp:143
 ErrorOr<std::unique_ptr<MemoryBuffer>>
 MemoryBuffer::getFileOrSTDIN(const Twine &Filename, int64_t FileSize,
                              bool RequiresNullTerminator) {
----------------
If you really feel there is a need for a second overload, this version should call the new version, with a do-nothing function of some kind.


================
Comment at: llvm/lib/Support/MemoryBuffer.cpp:155
+    const Twine &Filename,
+    std::function<std::unique_ptr<MemoryBuffer>(void)> NotRedirectedInput,
+    int64_t FileSize, bool RequiresNullTerminator) {
----------------
How about `OnUnredirectedInput`?


================
Comment at: llvm/unittests/Support/MemoryBufferTest.cpp:304
+        << "Reading from tty should have exited with exit code 1";
+    ASSERT_EQ(TestForChange, 1) << "Callback not called";
+    TestForChange = 0;
----------------
abrachet wrote:
> This test is actually failing, it doesn't change the int, but it does print "message" according to EXPECT_EXIT, I also tried other things like calling exit from inside the lamda with different exit status, and and it picks up that it wasn't exit code 1. So the ChangeFunc's operator() is being called, but TestForChange is still equal to 0 gtest says. I hope its just  a silly mistake with the lambda, but surely the compiler would have complained. I'm really at a loss for this one. Any ideas?
Does this need to be EXPECT_EXIT with your updated version?

I suspect the problem previously was the call of std::exit - I think EXPECT_EXIT spawns a separate process to run the test, which means that the state used in the lambda will be unrelated to that used in the code under test.


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

https://reviews.llvm.org/D63859