[PATCH] D143110: [mlgo] Make InteractiveModelRunner actually work with named pipes

[Kazu Hirata via Phabricator via llvm-commits]

kazu accepted this revision.
kazu added a comment.
This revision is now accepted and ready to land.

LGTM.  Just minor comments about sticking to the `FileSystem.h` API and removing a couple of `#include`s.



================
Comment at: llvm/include/llvm/Analysis/InteractiveModelRunner.h:19
 #include "llvm/Support/raw_ostream.h"
+#include <istream>
 #include <system_error>
----------------
If you are lowering things down to `sys::fs::file_t`, you probably don't need this!?


================
Comment at: llvm/lib/Analysis/InteractiveModelRunner.cpp:18
 #include "llvm/Support/raw_ostream.h"
+#include <unistd.h>
 
----------------
You should be able to remove this if you can switch `::read` to `sys::fs::readNativeFile` below.


================
Comment at: llvm/lib/Analysis/InteractiveModelRunner.cpp:78
+    auto Read =
+        ::read(Inbound, Buff + InsPoint, OutputBuffer.size() - InsPoint);
     if (Read < 0) {
----------------
Can you use ` sys::fs::readNativeFile` for consistency with `sys::fs::openFileForRead` and `sys::fs::closeFile`?


================
Comment at: llvm/unittests/Analysis/MLModelRunnerTest.cpp:180
+        char Chr = 0;
+        auto Read = ::read(FromCompiler, &Chr, 1);
+        EXPECT_GE(Read, 0);
----------------
Likewise, can you use `sys::fs::readNativeFile` for consistency with `sys::fs::openFileForRead`?


================
Comment at: llvm/unittests/Analysis/MLModelRunnerTest.cpp:204
       while (InsPt < ToRead) {
-        auto Read = FromCompiler.read(Buff + InsPt, ToRead - InsPt);
+        auto Read = ::read(FromCompiler, Buff + InsPt, ToRead - InsPt);
         EXPECT_GE(Read, 0);
----------------
Likewise.


================
Comment at: llvm/unittests/Analysis/MLModelRunnerTest.cpp:215
     char Chr = 0;
-    while (FromCompiler.read(&Chr, 1) == 0) {
+    while (::read(FromCompiler, &Chr, 1) == 0) {
     }
----------------
Likewise.


================
Comment at: llvm/unittests/Analysis/MLModelRunnerTest.cpp:232
     FullyRead();
-    while (FromCompiler.read(&Chr, 1) == 0) {
+    while (::read(FromCompiler, &Chr, 1) == 0) {
     }
----------------
Likewise.


================
Comment at: llvm/unittests/Analysis/MLModelRunnerTest.cpp:243
     ToCompiler.flush();
+    ::close(FromCompiler);
   });
----------------
Can you use `sys::fs::closeFile` here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143110