[PATCH] D98426: [llvm-objcopy][Support] move writeToStream helper function to Support.

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

In D98426#2635865 <https://reviews.llvm.org/D98426#2635865>, @avl wrote:

> addressed comments.
>
> I did not do two changes yet:
>
> - renaming writeToStream into writeToOutput.
> - moving writeToStream/writeToOutput into the other file.
>
> It seems to me that writeToStream clearly shows that we write into the stream.
> That is true that real writing happens inside Write argument. But there is no any
> possibility to write into other output than subclass of raw_ostream. So,
> when we call writeToStream we always write into the some stream.
>
> It also seems natural to have writeToStream into the raw_ostream.h/cpp since this
> function is for streams which are subclusses of raw_ostream. And all these subclusses
> are defined in raw_ostream.h/cpp.
>
> If you still think that above changes(rename writeToStream into writeToOutput, move
> writeToStream/writeToOutput into other file) should be done I would do this in next update.

I'm not too fussed about which file, as I don't have a good alternative location for it. I do think we should do the function renaming however. When I see `writeToStream`, I expect to pass in a stream to write to, but the function signature doesn't take a stream. `writeToOutput` meanwhile indicates the function will write to some specified output, which is a higher level thing.

There is of course a completely different approach, which would avoid this issue, namely to change the function to just create and return the stream and then leave the clients to pass the stream to their write functions. If you decided to switch to this approach, you'd probably want a new stream class derived from raw_fd_ostream which does the tempfile stuff itself, in the constructor/destructor, so that clients don't need t worry about that logic.



================
Comment at: llvm/unittests/Support/raw_ostream_test.cpp:509
+                    }),
+      FailedWithMessage("'/_bad/_path': No such file or directory"));
+}
----------------
Hmmm, on further reflection, I bet this will fail on some systems, because the spelling/capitalization etc of this message is dependent on the c++ standard library implementation (see various recent attempts to solve this problem in lit). You might need to do something like get the expected message from a local instantiation of the std::errc that matches and use that to compute the expected output.


================
Comment at: llvm/unittests/Support/raw_ostream_test.cpp:513
+TEST(raw_ostreamTest, writeToDevNull) {
+  bool devNullIsUsed = false;
+
----------------



================
Comment at: llvm/unittests/Support/raw_ostream_test.cpp:518
+                    [&](raw_ostream &Out) -> Error {
+                      Out << "HelloWorld";
+                      devNullIsUsed =
----------------
You could probably delete this line, since it doesn't actually do anything.


================
Comment at: llvm/unittests/Support/raw_ostream_test.cpp:526
+
+  ASSERT_TRUE(devNullIsUsed);
+}
----------------
You can use `EXPECT_TRUE` here, because this value being incorrect doesn't prevent the rest of the test from running (it's not really necessary, because this is the last statement, but it's a good habit to get into - use ASSERT... if and only if later statements rely on the asserted statement being true, e.g. ASSERT_TRUE that a pointer is not null, before dereferencing it in a later check.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98426