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

[James Henderson via Phabricator via llvm-commits]

jhenderson requested changes to this revision.
jhenderson added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/include/llvm/Support/raw_ostream.h:719-724
+/// A writeToStream helper creates an output stream, based on the specified
+/// \p OutputFileName: std::outs for the "-", raw_null_ostream for
+/// the "/dev/null", raw_fd_stream for other names. The final output
+/// file is atomically replaced with the temporary file after \p Write handler
+/// is finished. \p KeepOwnership is used to setting specified \p UserID and
+/// \p GroupID for the resulting file if writeToStream is called under /root.
----------------
There are a number of issues with the original comment, which we can fix whilst you're moving it. Please then reflow the comment to the 80 character limit. I don't understand what the last sentence of the comment is trying to say. I've suggested an alternative, but I'm not sure if it is right or not.

It seems to me like this should actually be called `writeToOutput`, not `writeToStream`. It is using the stream to write to some output, whilst the writing to the stream is actually inside the `Write` argument. I'm also not convinced that this belongs in the raw_ostream file, since it's really about writing to files (I think it maybe belongs in some File I/O file within the Support library), but I'm less concerned by that, since it is using a `raw_ostream`.


================
Comment at: llvm/unittests/Support/raw_ostream_test.cpp:492-499
+  if (Error Err = writeToStream(Path, [](raw_ostream &Out) -> Error {
+        Out << "HelloWorld";
+        return Error::success();
+      })) {
+    ADD_FAILURE() << "Error while writing to \'" << Path << "\'.";
+  } else {
+    checkFileData(Path, "HelloWorld");
----------------
I believe you actually want this approach. Similar applies to each of the others below.


================
Comment at: llvm/unittests/Support/raw_ostream_test.cpp:531-539
+  if (Error Err = writeToStream("/_bad/_path", [](raw_ostream &Out) -> Error {
+        Out << "HelloWorld";
+        return Error::success();
+      })) {
+    ASSERT_TRUE((bool)Err);
+    consumeError(std::move(Err));
+  } else {
----------------
This should be as suggested inline.


================
Comment at: llvm/unittests/Support/raw_ostream_test.cpp:543
+TEST(raw_ostreamTest, writeToDevNull) {
+  if (Error Err = writeToStream("/dev/null", [](raw_ostream &Out) -> Error {
+        Out << "HelloWorld";
----------------
Use `EXPECT_THAT_ERROR(..., Succeeded())` here and below too.


================
Comment at: llvm/unittests/Support/raw_ostream_test.cpp:544
+  if (Error Err = writeToStream("/dev/null", [](raw_ostream &Out) -> Error {
+        Out << "HelloWorld";
+        return Error::success();
----------------
Perhaps add something in this functor to show that it is actually run, and then check that thing after the call has finished.


================
Comment at: llvm/unittests/Support/raw_ostream_test.cpp:553
+  if (Error Err = writeToStream("-", [](raw_ostream &Out) -> Error {
+        Out << "HelloWorld";
+        return Error::success();
----------------
I'm not sure we should be writing to stdout directly here, unless you've got some way of capturing and redirecting stdout back so that you can check it. In fact, as things stand, the writeToStream function could just return `Error::success()` without even calling the underlying functor. You need to a) show that the raw_ostream in use is the stdout stream, and b) show that the functor is called.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98426