This is an archive of the discontinued LLVM Phabricator instance.

[llvm-objcopy][Support] move writeToStream helper function to Support.
ClosedPublic

Authored by avl on Mar 11 2021, 7:09 AM.

Details

Summary

writeToStream function is useful when it is necessary to create different kinds
of streams(based on stream name) and when we need to use a temporary file
while writing(which would be renamed into the resulting file in a success case).
This patch moves the writeToStream helper into the Support library.

Diff Detail

Event Timeline

avl created this revision.Mar 11 2021, 7:09 AM
avl requested review of this revision.Mar 11 2021, 7:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 11 2021, 7:09 AM
dblaikie accepted this revision.Mar 11 2021, 1:58 PM

Sounds good to me

This revision is now accepted and ready to land.Mar 11 2021, 1:58 PM
avl added a comment.Mar 12 2021, 2:10 AM

Thank you for the review!

jhenderson requested changes to this revision.Mar 17 2021, 3:09 AM
jhenderson added inline comments.
llvm/include/llvm/Support/raw_ostream.h
719–724

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.

llvm/unittests/Support/raw_ostream_test.cpp
493–500

I believe you actually want this approach. Similar applies to each of the others below.

532–540

This should be as suggested inline.

544

Use EXPECT_THAT_ERROR(..., Succeeded()) here and below too.

545

Perhaps add something in this functor to show that it is actually run, and then check that thing after the call has finished.

554

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.

This revision now requires changes to proceed.Mar 17 2021, 3:09 AM
avl updated this revision to Diff 331692.Mar 18 2021, 2:43 PM

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.

In 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.

llvm/unittests/Support/raw_ostream_test.cpp
509

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.

513
518

You could probably delete this line, since it doesn't actually do anything.

526

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.

avl added a comment.Mar 19 2021, 3:36 AM

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.

I thought about similar approach:

std::unique_ptr<raw_ostream> Stream = createStream(FileName);

Stream << "data to write";

But it makes error handling to be inconvenient. The subclass of raw_fd_ostream needs to return Error from it`s destructor.

raw_fd_ostream_sub::~raw_fd_ostream_sub () {
    Error Err = Temp->keep(OutputFileName);
}

So it is not clear how to get that error from the destructor of std::unique_ptr(which calls destructor of subclass of raw_fd_ostream).

avl updated this revision to Diff 331858.Mar 19 2021, 6:52 AM

addressed comments.

jhenderson accepted this revision.Mar 22 2021, 2:31 AM

LGTM, thanks!

llvm/unittests/Support/raw_ostream_test.cpp
487
This revision is now accepted and ready to land.Mar 22 2021, 2:31 AM
avl added a comment.Mar 22 2021, 2:35 AM

Thank you, for the review.

hokein added a subscriber: hokein.Jun 23 2023, 8:42 AM
hokein added inline comments.
llvm/lib/Support/raw_ostream.cpp
1003

is the all_exe mode intended here? (the clang-include-cleaner tool is using this API, I was debugging an issue where the file type of the modified file was changed executable).

Herald added a project: Restricted Project. · View Herald TranscriptJun 23 2023, 8:42 AM
avl added inline comments.Jun 23 2023, 9:13 AM
llvm/lib/Support/raw_ostream.cpp
1003

yep, It looks incorrectly to set it by default. It would be better to remove it or make configurable.

there is similar API llvm::writeFileAtomically. probably it would be better to unite both of this APIs.

hokein added inline comments.Jun 23 2023, 10:48 AM
llvm/lib/Support/raw_ostream.cpp
1003

yep, It looks incorrectly to set it by default. It would be better to remove it or make configurable.

thanks. I will sent out a patch.

there is similar API llvm::writeFileAtomically. probably it would be better to unite both of this APIs.

Sounds good to me. There is only 2 in-tree usages of llvm::writeFileAtomically API, I think we can even just remove this API.