[PATCH] D86913: Add raw_fd_stream that supports reading/seeking/writing

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/Support/raw_ostream.h:592-593
+  ///
+  /// On success, the number of bytes read is returned, and the file position is
+  // advanced by this number. On error, -1 is returned, and EC is set.
+  ssize_t read(char *Ptr, size_t Size);
----------------
This comment doesn't seem to line up with the interface, where it talks about setting `EC` on failure. I'm guessing this isn't the `EC` from the constructor, since there's no state involved, and theres' no other `EC` parameter!


================
Comment at: llvm/unittests/Support/raw_fd_stream_test.cpp:1
+//===- llvm/unittest/Support/raw_fd_stream_test.cpp - raw_fd_stream tests -===//
+//
----------------
A couple of other test cases I'd like to see, depending on how practical it is (I suspect it's not practical, and if so, don't worry).

1) An unseekable input (i.e. where `supportsSeeking` returns `false`).
2) An unseekable input, but where `EC` was already in an error state (and so the new error wasn't reported).


================
Comment at: llvm/unittests/Support/raw_fd_stream_test.cpp:38
+  SmallString<64> Path;
+  const char *ParentPath = getenv("RAW_FD_STREAM_TEST_FILE");
+  if (ParentPath) {
----------------
Is this environment variable ever going to be set? I'm not sure what the purpose of the extra logic is...


================
Comment at: llvm/unittests/Support/raw_fd_stream_test.cpp:43
+    int FD;
+    ASSERT_NO_ERROR(sys::fs::createTemporaryFile("foo", "bar", FD, Path));
+    setenv("RAW_FD_STREAM_TEST_FILE", Path.c_str(), true);
----------------
I'm not sure I understand the need for the macro. Can this not just be the following?

`ASSERT_FALSE(sys::fs::createTemporaryFile("foo", "bar", FD, PATH));`

(with optional explicit `operator bool` call, if needed)


================
Comment at: llvm/unittests/Support/raw_fd_stream_test.cpp:47
+  FileRemover Cleanup(Path);
+  std::error_code ec;
+  raw_fd_stream OS(Path, ec);
----------------
`ec` -> `EC` for LLVM style.


================
Comment at: llvm/unittests/Support/raw_fd_stream_test.cpp:77
+  {
+    std::error_code ec;
+    raw_fd_stream OS("-", ec);
----------------
Ditto ec -> EC here too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86913