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

[Alex Brachet via Phabricator via libc-commits]

abrachet added inline 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();
----------------
Needs brackets and no comment


================
Comment at: libc/utils/testutils/FDReaderUnix.cpp:32-35
+  const ssize_t chunkSize = 4096 * 4;
+  // chunkSize stolen from MemoryBuffer.cpp and getMemoryBufferForStream
+
+  char buffer[chunkSize];
----------------
`const` -> `constexpr`
I don't think you need to justify the chunk size with the comment.


================
Comment at: libc/utils/testutils/FDReaderUnix.cpp:40
+
+  std::cout << str << std::endl;
+
----------------
Probably left over from testing.


================
Comment at: libc/utils/testutils/FDReaderUnix.cpp:42-43
+
+  for (;;) {
+    bytesRead = read(pipefd[0], buffer, chunkSize);
+    if (bytesRead > 0) {
----------------
This loop could maybe be
`for (ssize_t bytesRead; (bytesRead = ::read(...)); )` And then remove the `bytesRead == 0` check


================
Comment at: libc/utils/testutils/FDReaderUnix.cpp:45
+    if (bytesRead > 0) {
+      pipeStr.append(buffer);
+    } else if (bytesRead == 0) {
----------------
Read will not null terminate the buffer, the input probably won't have a `'\0'` in it either. I think `pipeStr.insert(pipeStr.end(), buffer, bytesRead)` or similar is safer.


================
Comment at: libc/utils/testutils/FDReaderUnix.cpp:54
+
+  return pipeStr.compare(inputStr) == 0;
+  // llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> bufOrErr =
----------------
I think `pipeStr == inputStr` is more clear
And remove the comment below


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91665