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

[Nicolas Guillemot via Phabricator via llvm-commits]

nlguillemot marked 2 inline comments as done.
nlguillemot added inline comments.


================
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.
----------------
mkitzan wrote:
> 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) {}`.
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


================
Comment at: llvm/include/llvm/Support/raw_ostream.h:645
+    OutStream << Value;
+    if (Delim != 0)
+      OutStream << Delim;
----------------
mkitzan wrote:
> 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 ]]).
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


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

https://reviews.llvm.org/D78795