This is an archive of the discontinued LLVM Phabricator instance.

[Support] Add raw_ostream_iterator: ostream_iterator for raw_ostream.
ClosedPublic

Authored by nlguillemot on Apr 23 2020, 11:24 PM.

Details

Summary

Adds a class raw_ostream_iterator that behaves like
std::ostream_iterator, but can be used with raw_ostream.
This is useful for using raw_ostream with std algorithms.

For example, it can be used to output std containers as follows:

std::vector<int> V = { 1, 2, 3 };
std::copy(V.begin(), V.end(), raw_ostream_iterator<int>(outs(), ", "));
// Output: "1, 2, 3, "

The API tries to follow std::ostream_iterator as closely as is
practically possible.

Diff Detail

Event Timeline

This is patch is mainly useful for enabling some of the refactors that happen in the next patch here: https://reviews.llvm.org/D78796

nlguillemot edited the summary of this revision. (Show Details)Apr 23 2020, 11:43 PM
nlguillemot updated this revision to Diff 259817.EditedApr 23 2020, 11:45 PM

Some example code and the commit message were showing an example of printing a vector<double>. It turns out that doubles don't print so beautifully by default, so switch the example to use int instead to avoid being misleading.

Herald added a project: Restricted Project. · View Herald TranscriptApr 26 2020, 11:35 AM
dblaikie accepted this revision.Apr 27 2020, 10:47 PM

Maybe some more test coverage (empty range at least) but otherwise looks good.

Maybe hold of on committing this until some of the rest of the patch sequence has been reviewed/approved, in case it doesn't end up being needed.

llvm/include/llvm/Support/raw_ostream.h
637

Could use a non-static data member initializer for Delim = nullptr, and not bother initializing it here, perhaps? (I don't feel super strongly about this)

llvm/unittests/Support/raw_ostream_test.cpp
358–360

FWIW (again, don't feel strongly) I'd probably use an explicit scope rather than an explicit call to flush here. *shrug*

This revision is now accepted and ready to land.Apr 27 2020, 10:47 PM
dblaikie added inline comments.Apr 27 2020, 10:49 PM
llvm/unittests/Support/raw_ostream_test.cpp
355–371

If you could simplify the testing to not use another whole container type and standard algorithm, that'd probably be good, in my opinion.

It'd make the tests more explicit/obvious about the surface area they're testing. You could test the empty case, and writing a few elements to the output iterator.

(this I feel a bit more strongly about than the other two comments)

nlguillemot marked an inline comment as done.
  • Added some basic tests that use raw_ostream_iterator manually instead of through std::copy.
  • Replaced manual calls to flush() in the test with an artificial scope (takes advantage of the fact that ~raw_string_ostream() calls flush())
  • Added a test for std::copy with an empty range.
nlguillemot marked 2 inline comments as done.Apr 28 2020, 10:21 AM

Maybe hold of on committing this until some of the rest of the patch sequence has been reviewed/approved, in case it doesn't end up being needed.

I agree.

Thanks for the review @dblaikie !

mkitzan added a subscriber: mkitzan.EditedMay 7 2020, 9:50 AM

Looks like a good helper class. Just some minor nits. (haven't looked into the tests just yet)

llvm/include/llvm/Support/raw_ostream.h
629–637

Alternatively, could collapse into one ctor with a default argument for Delim.
raw_ostream_iterator(raw_ostream &Stream, const CharT * Delim = nullptr) : OutStream(Stream), Delim(Delim) {}.

637

Yeah, changing L637 to : OutStream(Stream) {} and L612 to const CharT *Delim = nullptr would comply better with CppCoreGuidelines C.48, but it's still a nit.

645

Also a nit, could just be if (Delim) or if you really want to explicitly show the comparison if (Delim != nullptr) (CppCoreGuideline ES.87).

nlguillemot marked 2 inline comments as done.May 7 2020, 9:57 AM
nlguillemot added inline comments.
llvm/include/llvm/Support/raw_ostream.h
629–637

I agree that would be cleaner, but I kept it as two constructors to match the specification here: https://en.cppreference.com/w/cpp/iterator/ostream_iterator/ostream_iterator

645

it's kind of silly, but I wrote it this way to match the code here as closely as possible: https://en.cppreference.com/w/cpp/iterator/ostream_iterator/operator%3D

Rebased the patch onto main.

Reverted since there are some build errors on Windows bots. Here's the error log:

[739/841] Building CXX object unittests\Support\CMakeFiles\SupportTests.dir\raw_ostream_test.cpp.obj
FAILED: unittests/Support/CMakeFiles/SupportTests.dir/raw_ostream_test.cpp.obj 
C:\PROGRA~2\MICROS~3\2019\PROFES~1\VC\Tools\MSVC\1428~1.293\bin\Hostx64\x64\cl.exe  /nologo /TP -DGTEST_HAS_RTTI=0 -DGTEST_HAS_TR1_TUPLE=0 -DUNICODE -D_CRT_NONSTDC_NO_DEPRECATE -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_SECURE_NO_DEPRECATE -D_CRT_SECURE_NO_WARNINGS -D_HAS_EXCEPTIONS=0 -D_SCL_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_WARNINGS -D_UNICODE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Iunittests\Support -IC:\b\slave\clang-x64-windows-msvc\llvm-project\llvm\unittests\Support -Iinclude -IC:\b\slave\clang-x64-windows-msvc\llvm-project\llvm\include -IC:\b\slave\clang-x64-windows-msvc\llvm-project\llvm\utils\unittest\googletest\include -IC:\b\slave\clang-x64-windows-msvc\llvm-project\llvm\utils\unittest\googlemock\include /DWIN32 /D_WINDOWS   /Zc:inline /Zc:__cplusplus /Zi /Zc:strictStrings /Oi /Zc:rvalueCast /bigobj /W4 -wd4141 -wd4146 -wd4244 -wd4267 -wd4291 -wd4351 -wd4456 -wd4457 -wd4458 -wd4459 -wd4503 -wd4624 -wd4722 -wd4100 -wd4127 -wd4512 -wd4505 -wd4610 -w
d4510 -wd4702 -wd4245 -wd4706 -wd4310 -wd4701 -wd4703 -wd4389 -wd4611 -wd4805 -wd4204 -wd4577 -wd4091 -wd4592 -wd4319 -wd4709 -wd4324 -w14062 -we4238 /Gw /MD /O2 /Ob2  /EHs-c- /GR- -UNDEBUG -std:c++14 /showIncludes /Founittests\Support\CMakeFiles\SupportTests.dir\raw_ostream_test.cpp.obj /Fdunittests\Support\CMakeFiles\SupportTests.dir\ /FS -c C:\b\slave\clang-x64-windows-msvc\llvm-project\llvm\unittests\Support\raw_ostream_test.cpp
C:\Program Files (x86)\Microsoft Visual Studio\2019\Professional\VC\Tools\MSVC\14.28.29333\include\xutility(1492): error C2280: 'llvm::raw_ostream_iterator<char,char> &llvm::raw_ostream_iterator<char,char>::operator =(const llvm::raw_ostream_iterator<char,char> &)': attempting to reference a deleted function
C:\b\slave\clang-x64-windows-msvc\llvm-project\llvm\include\llvm/Support/raw_ostream.h(776): note: compiler has generated 'llvm::raw_ostream_iterator<char,char>::operator =' here
C:\b\slave\clang-x64-windows-msvc\llvm-project\llvm\include\llvm/Support/raw_ostream.h(776): note: 'llvm::raw_ostream_iterator<char,char> &llvm::raw_ostream_iterator<char,char>::operator =(const llvm::raw_ostream_iterator<char,char> &)': function was implicitly deleted because 'llvm::raw_ostream_iterator<char,char>' has a data member 'llvm::raw_ostream_iterator<char,char>::OutStream' of reference type
C:\b\slave\clang-x64-windows-msvc\llvm-project\llvm\include\llvm/Support/raw_ostream.h(731): note: see declaration of 'llvm::raw_ostream_iterator<char,char>::OutStream'
C:\Program Files (x86)\Microsoft Visual Studio\2019\Professional\VC\Tools\MSVC\14.28.29333\include\xutility(4429): note: see reference to function template instantiation 'void std::_Seek_wrapped<_OutIt,_OutIt>(_Iter &,_UIter &&)' being compiled
       with
       [
           _OutIt=llvm::raw_ostream_iterator<char,char>,
           _Iter=llvm::raw_ostream_iterator<char,char>,
           _UIter=llvm::raw_ostream_iterator<char,char>
       ]
C:\b\slave\clang-x64-windows-msvc\llvm-project\llvm\unittests\Support\raw_ostream_test.cpp(456): note: see reference to function template instantiation '_OutIt std::copy<InputIt,llvm::raw_ostream_iterator<char,char>>(_InIt,_InIt,_OutIt)' being compiled
       with
       [
           _OutIt=llvm::raw_ostream_iterator<char,char>,
           InputIt=std::_Vector_iterator<std::_Vector_val<std::_Simple_types<char>>>,
           _InIt=std::_Vector_iterator<std::_Vector_val<std::_Simple_types<char>>>
       ]
C:\b\slave\clang-x64-windows-msvc\llvm-project\llvm\unittests\Support\raw_ostream_test.cpp(475): note: see reference to function template instantiation 'std::string `anonymous-namespace'::iterator_str<char,std::_Vector_iterator<std::_Vector_val<std::_Simple_types<_Elem>>>>(InputIt,InputIt)' being compiled
       with
       [
           _Elem=char,
           InputIt=std::_Vector_iterator<std::_Vector_val<std::_Simple_types<char>>>
       ]
C:\Program Files (x86)\Microsoft Visual Studio\2019\Professional\VC\Tools\MSVC\14.28.29333\include\xutility(1492): error C2280: 'llvm::raw_ostream_iterator<int,char> &llvm::raw_ostream_iterator<int,char>::operator =(const llvm::raw_ostream_iterator<int,char> &)': attempting to reference a deleted function
C:\b\slave\clang-x64-windows-msvc\llvm-project\llvm\include\llvm/Support/raw_ostream.h(776): note: compiler has generated 'llvm::raw_ostream_iterator<int,char>::operator =' here
C:\b\slave\clang-x64-windows-msvc\llvm-project\llvm\include\llvm/Support/raw_ostream.h(776): note: 'llvm::raw_ostream_iterator<int,char> &llvm::raw_ostream_iterator<int,char>::operator =(const llvm::raw_ostream_iterator<int,char> &)': function was implicitly deleted because 'llvm::raw_ostream_iterator<int,char>' has a data member 'llvm::raw_ostream_iterator<int,char>::OutStream' of reference type
C:\b\slave\clang-x64-windows-msvc\llvm-project\llvm\include\llvm/Support/raw_ostream.h(731): note: see declaration of 'llvm::raw_ostream_iterator<int,char>::OutStream'
C:\Program Files (x86)\Microsoft Visual Studio\2019\Professional\VC\Tools\MSVC\14.28.29333\include\xutility(4429): note: see reference to function template instantiation 'void std::_Seek_wrapped<_OutIt,_OutIt>(_Iter &,_UIter &&)' being compiled
       with
       [
           _OutIt=llvm::raw_ostream_iterator<int,char>,
           _Iter=llvm::raw_ostream_iterator<int,char>,
           _UIter=llvm::raw_ostream_iterator<int,char>
       ]
C:\b\slave\clang-x64-windows-msvc\llvm-project\llvm\unittests\Support\raw_ostream_test.cpp(466): note: see reference to function template instantiation '_OutIt std::copy<InputIt,llvm::raw_ostream_iterator<int,char>>(_InIt,_InIt,_OutIt)' being compiled
       with

Here's a repro for the compile error on Windows: https://godbolt.org/z/5EdWKn