[libc-commits] [PATCH] D91665: [libc] Make more of the libc unit testing llvm independent

[Michael Jones via Phabricator via libc-commits]

michaelrj added a comment.

submit comments



================
Comment at: libc/utils/testutils/FDReaderUnix.cpp:20-23
   if (::pipe(pipefd))
-    llvm::report_fatal_error("pipe(2) failed");
+    // llvm::report_fatal_error("pipe(2) failed");
+    std::cerr << "pipe(2) failed";
+  abort();
----------------
abrachet wrote:
> Needs brackets and no comment
I feel very silly for missing that one. That was why the test was failing immediately.


================
Comment at: libc/utils/testutils/FDReaderUnix.cpp:42-43
+
+  for (;;) {
+    bytesRead = read(pipefd[0], buffer, chunkSize);
+    if (bytesRead > 0) {
----------------
abrachet wrote:
> This loop could maybe be
> `for (ssize_t bytesRead; (bytesRead = ::read(...)); )` And then remove the `bytesRead == 0` check
I made one small change to your code, `bytesRead` needs to be signed because `read` uses negative numbers for errors


================
Comment at: libc/utils/testutils/FDReaderUnix.cpp:54
+
+  return pipeStr.compare(inputStr) == 0;
+  // llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> bufOrErr =
----------------
abrachet wrote:
> I think `pipeStr == inputStr` is more clear
> And remove the comment below
oops, as I'm sure you guessed I forgot to clean up my code before submitting.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91665