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

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

dexonsmith accepted this revision.
dexonsmith added a reviewer: bkramer.
dexonsmith added a subscriber: bkramer.
dexonsmith added a comment.
This revision is now accepted and ready to land.

LGTM, with a couple of comments inline (e.g., I might leave `str()` alone for now, delete all callers, and then delete it entirely, but I'm okay either way).

> plus the potential (small) performance+flexibility loss about not being able to buffer to avoid std::string's eager null terminator writes

Users can still buffer for that case, they'd just need to manually flush. But most of the time, better to use a likely-big-enough `SmallString` and a `raw_svector_ostream` and then convert to a `std::string` at the end.



================
Comment at: llvm/include/llvm/Support/raw_ostream.h:640
   explicit raw_string_ostream(std::string &O) : OS(O) {
     SetUnbuffered();
   }
----------------
Interesting; I didn't realize this was already here. Looks like it's relatively recent, from 65b13610a5226b84889b923bae884ba395ad084d, but that means we've already analyzed and/or accepted any perf regressions.

@bkramer, any specific reason you left the `flush()`s around? Or just didn't have time to finish aligning with `raw_svector_ostream`?


================
Comment at: llvm/include/llvm/Support/raw_ostream.h:642
   }
-  ~raw_string_ostream() override;
+  ~raw_string_ostream() override = default;
 
----------------
I think you can just delete this line entirely now.


================
Comment at: llvm/include/llvm/Support/raw_ostream.h:644-645
 
-  /// 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; }
 
----------------
I wonder if this API should be deleted entirely? (not in this commit) -- in which case, could land just the change to the destructor here, then delete callers, then delete this.


================
Comment at: llvm/unittests/Support/raw_ostream_test.cpp:23-30
   std::string res;
   {
     llvm::raw_string_ostream OS(res);
     OS.SetBuffered();
     OS << Value;
+    OS.flush();
   }
----------------
I'd suggest dropping the scope as well since it's not doing anything anymore.


================
Comment at: llvm/unittests/Support/raw_ostream_test.cpp:146
+  OS.flush();
   EXPECT_EQ("hello1world", OS.str());
 }
----------------
You should drop the `.str()` on this line as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115421