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

[Marshall Clow via cfe-commits]

mclow.lists added inline comments.

================
Comment at: include/experimental/iterator:62
@@ +61,3 @@
+
+_LIBCPP_BEGIN_NAMESPACE_LFTS
+
----------------
EricWF wrote:
> Wrong namespace. This gives `std::experimental::fundamentals_v2`. This brings up the bigger question of how we want to manage two TS's.
I think that the right thing is to flip all the LFTS over to v2.

================
Comment at: include/experimental/iterator:94
@@ +93,3 @@
+
+    ostream_joiner& operator*()     _NOEXCEPT { return *this; }
+    ostream_joiner& operator++()    _NOEXCEPT { return *this; }
----------------
EricWF wrote:
> Unrelated note: Are we ever going to stop guarding `noexcept` behind a macro?
Maybe. Depends on how much work it turns out to be. 

I don't want to make that decision here, though.

================
Comment at: test/std/experimental/iterator/nothing_to_do.pass.cpp:1
@@ +1,2 @@
+//===----------------------------------------------------------------------===//
+//
----------------
EricWF wrote:
> LIT will only emit a warning if all sub directories are empty as well. Do we need this file for `testit`^
Well, if anyone was still running `testit`, yes.
But it's a nice simple test. Does including the header file actually work?

See `test/std/experimental/optional` or `string_view` for similar tests.

================
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)
----------------
EricWF wrote:
> Can you test that `joiner` has the expected type?
Sure.

================
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(); }
----------------
EricWF wrote:
> 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<<`.
I'll add that as well.



http://reviews.llvm.org/D16605