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

[stephan.yichao.zhao via Phabricator via llvm-commits]

stephan.yichao.zhao marked an inline comment as done.
stephan.yichao.zhao 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);
----------------
jhenderson wrote:
> 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!
Updated comments. 


================
Comment at: llvm/unittests/Support/raw_fd_stream_test.cpp:1
+//===- llvm/unittest/Support/raw_fd_stream_test.cpp - raw_fd_stream tests -===//
+//
----------------
jhenderson wrote:
> 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).
Looked into gmock a bit. To mock supportsSeeking, the code under test needs to rewrite to use either virtual method or templete: https://github.com/google/googletest/blob/master/googlemock/docs/cook_book.md#MockingNonVirtualMethods

Another way to test it is like this. At raw_fd_ostream::raw_fd_ostream, at a place where SupportsSeeking is set, we do

  if GlobalMockValue is set:
    SupportsSeeking =  GlobalMockValue
  else
    SupportsSeeking = ...

Here GlobalMockValue could be a global pointer. And the unit test can set it. EC can be tested in the same way.

But this also needs to change the code under test. Did any other LLVM test code do something similar?


================
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) {
----------------
jhenderson wrote:
> Is this environment variable ever going to be set? I'm not sure what the purpose of the extra logic is...
This code refers to the test raw_pwrite_ostreamTest from llvm/unittests/Support/raw_pwrite_stream_test.cpp

That test has a death test at the end. This new test does not have death test. I removed unrelated code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86913