[PATCH] D115421: [Support] No longer require .flush()ing raw_string_ostream

[Logan Smith via Phabricator via llvm-commits]

logan-5 created this revision.
logan-5 added a reviewer: dexonsmith.
logan-5 added a project: LLVM.
Herald added subscribers: ctetreau, hiraditya.
logan-5 requested review of this revision.
Herald added a subscriber: llvm-commits.

This solves some problems in https://reviews.llvm.org/D115374 by eliminating the need for `.flush()` calls before returning the underlying strings. It also brings `raw_string_ostream` closer into parity with `raw_svector_ostream`, which does not require (nor even allow, see below) flushing.

I tried `void flush() = delete;` inside `raw_string_ostream` for parity with `raw_svector_ostream`, but ended up with around 750 call sites that needed to be manually inspected/fixed. (And if that's any indication, I think we can be sure the `=delete` //will// break lots of downstream code.) A great many of them were call sites that followed the `OS.flush(); return Underlying;` pattern I originally put forth in D115374 <https://reviews.llvm.org/D115374>. @dexonsmith, you mentioned that you weren't sure if the manual `.flush()`es were even all that terrible; my inspection of maybe a quarter of the 750 call sites suggests very strong precedent for this pattern already throughout LLVM. With this, plus the potential (small) performance+flexibility loss about not being able to buffer to avoid `std::string`'s eager null terminator writes, I admit I'm back on the fence about removing `flush()`.

(On the other hand, we could just not `=delete` flush, i.e. still allow it but no longer require it, which is effectively what this diff accomplishes.)


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D115421

Files:
  llvm/include/llvm/Support/raw_ostream.h
  llvm/lib/Support/raw_ostream.cpp


Index: llvm/lib/Support/raw_ostream.cpp
===================================================================
--- llvm/lib/Support/raw_ostream.cpp
+++ llvm/lib/Support/raw_ostream.cpp
@@ -937,10 +937,6 @@
 //  raw_string_ostream
 //===----------------------------------------------------------------------===//
 
-raw_string_ostream::~raw_string_ostream() {
-  flush();
-}
-
 void raw_string_ostream::write_impl(const char *Ptr, size_t Size) {
   OS.append(Ptr, Size);
 }
Index: llvm/include/llvm/Support/raw_ostream.h
===================================================================
--- llvm/include/llvm/Support/raw_ostream.h
+++ llvm/include/llvm/Support/raw_ostream.h
@@ -622,6 +622,9 @@
 
 /// A raw_ostream that writes to an std::string.  This is a simple adaptor
 /// class. This class does not encounter output errors.
+/// raw_string_ostream operates without a buffer, delegating all memory
+/// management to the std::string. Thus the std::string is always up-to-date,
+/// may be used directly and there is no need to call flush().
 class raw_string_ostream : public raw_ostream {
   std::string &OS;
 
@@ -636,14 +639,10 @@
   explicit raw_string_ostream(std::string &O) : OS(O) {
     SetUnbuffered();
   }
-  ~raw_string_ostream() override;
+  ~raw_string_ostream() override = default;
 
-  /// Flushes the stream contents to the target string and returns  the string's
-  /// reference.
-  std::string& str() {
-    flush();
-    return OS;
-  }
+  /// Returns the string's reference.
+  std::string& str() { return OS; }
 
   void reserveExtraSpace(uint64_t ExtraSize) override {
     OS.reserve(tell() + ExtraSize);


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D115421.393015.patch
Type: text/x-patch
Size: 1634 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20211209/21f3d2af/attachment.bin>