[PATCH] D16605: Implement `std::experimental::ostream_joiner`

[Eric Fiselier via cfe-commits]

EricWF added a comment.

- Please add the header to `test/libcxx/double_include.sh.cpp`
- Please add a `_LIBCPP_VERSION` test in `test/libcxx/experimental/iterator`
- The `ostream_joiner` type and it's members should be given visibility attributes.


================
Comment at: include/experimental/iterator:62
@@ +61,3 @@
+
+_LIBCPP_BEGIN_NAMESPACE_LFTS
+
----------------
Wrong namespace. This gives `std::experimental::fundamentals_v2`. This brings up the bigger question of how we want to manage two TS's.

================
Comment at: include/experimental/iterator:64
@@ +63,3 @@
+
+template <class _Delim, class _CharT = char, class _Traits = char_traits<_CharT>>
+class ostream_joiner {
----------------
Is `_Delim` allowed to be a reference? We might get better diagnostics if we static assert some requirements on `_Delim`.

================
Comment at: include/experimental/iterator:85
@@ +84,3 @@
+    template<typename _Tp>
+    ostream_joiner& operator=(const _Tp& __v)
+    {
----------------
People are going to try and use this as an assignment operator. We either need to static assert in the body, or disable the operator entirely.
I'm not sure which is better. It depends on if we want a generated operator=. 

================
Comment at: include/experimental/iterator:94
@@ +93,3 @@
+
+    ostream_joiner& operator*()     _NOEXCEPT { return *this; }
+    ostream_joiner& operator++()    _NOEXCEPT { return *this; }
----------------
Unrelated note: Are we ever going to stop guarding `noexcept` behind a macro?

================
Comment at: test/std/experimental/iterator/nothing_to_do.pass.cpp:1
@@ +1,2 @@
+//===----------------------------------------------------------------------===//
+//
----------------
LIT will only emit a warning if all sub directories are empty as well. Do we need this file for `testit`^

================
Comment at: test/std/experimental/iterator/ostream.joiner/ostream.joiner.creation/make_ostream_joiner.pass.cpp:35
@@ +34,3 @@
+    std::basic_stringstream<CharT, Traits> sstream;
+    auto joiner = exp::make_ostream_joiner(sstream, d);
+    while (first != last)
----------------
Can you test that `joiner` has the expected type?

================
Comment at: test/std/experimental/iterator/ostream.joiner/ostream.joiner.ops/ostream_joiner.op.assign.pass.cpp:37
@@ +36,3 @@
+std::basic_ostream<_CharT, _Traits>&
+operator<<(std::basic_ostream<_CharT, _Traits>& os, const mutating_delimiter &d)
+{ return os << d.get(); }
----------------
Nice test. Is there a reason to pass the delimiter as `const` here? It seems equally valid that the mutating delimiter has a non-const `operator<<`.


http://reviews.llvm.org/D16605