[PATCH] D78795: [Support] Add raw_ostream_iterator: ostream_iterator for raw_ostream.

[Michael Kitzan via Phabricator via llvm-commits]

mkitzan added a comment.

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



================
Comment at: llvm/include/llvm/Support/raw_ostream.h:637
+  raw_ostream_iterator(raw_ostream &Stream)
+      : OutStream(Stream), Delim(nullptr) {}
+
----------------
dblaikie wrote:
> 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)
Yeah, changing L637 to `: OutStream(Stream) {}` and L612 to `const CharT *Delim = nullptr` would comply better with [[ http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c48-prefer-in-class-initializers-to-member-initializers-in-constructors-for-constant-initializers | CppCoreGuidelines C.48 ]], but it's still a nit.


================
Comment at: llvm/include/llvm/Support/raw_ostream.h:629-637
+  raw_ostream_iterator(raw_ostream &Stream, const CharT *Delim)
+      : OutStream(Stream), Delim(Delim) {}
+
+  /// Constructs the iterator with \p Stream as the associated stream and a null
+  /// pointer as the delimiter.
+  ///
+  /// \param Stream The output stream to be accessed by this iterator.
----------------
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) {}`.


================
Comment at: llvm/include/llvm/Support/raw_ostream.h:645
+    OutStream << Value;
+    if (Delim != 0)
+      OutStream << Delim;
----------------
Also a nit, could just be `if (Delim)` or if you really want to explicitly show the comparison `if (Delim != nullptr)` ([[ http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c48-prefer-in-class-initializers-to-member-initializers-in-constructors-for-constant-initializers | CppCoreGuideline ES.87 ]]).


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

https://reviews.llvm.org/D78795