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

[logan-5]

Summary

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. @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.)

[logan-5]

Fixed some tests--had to add manual .flush()es back in to some tests that were testing general flushing behavior of raw_ostream, but using raw_string_ostream for convenience.

[logan-5]

Formatting.

[dexonsmith]

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.

llvm/include/llvm/Support/raw_ostream.h

640

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?

642

I think you can just delete this line entirely now.

644–645

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.

llvm/unittests/Support/raw_ostream_test.cpp

23–30

I'd suggest dropping the scope as well since it's not doing anything anymore.

146

You should drop the .str() on this line as well.

[logan-5]

Addressed comments

[logan-5]

llvm/include/llvm/Support/raw_ostream.h

644–645

N.B. raw_svector_ostream has str() that returns StringRef, which appears to be useful in places. We could keep this for API parity, but change it to return StringRef to discourage using it to initialize std::strings.

[dexonsmith]

llvm/include/llvm/Support/raw_ostream.h

644–645

Sure, fine by me if there are places where the OS.str() actually helps. I kind of suspect there aren't any though, and this was just to make flush+ref a one-liner.

raw_svector_ostream::str() is more useful since SmallVectorImpl<char> doesn't have an implicit conversion to StringRef, whereas std::string does.

[logan-5]

To be quite honest, in regards to the big .str() refactor (whether changing its type or deleting it), I feel I may have started to bite off more than I can chew here. There are at least a few thousand call sites of .str() throughout LLVM and its subprojects, and as just a very occasional contributor who will be a distant stranger to most of them, I don't feel equipped or qualified to tackle auditing and fixing them myself. (Not to mention that it's orders of magnitude more work than I had planned to contribute with my initial patch.) I just wanted to optimize a few returns, and now we're talking about making a huge number of changes across the entire repo. I'm happy to bring this current patch through to landing, but when it comes to the big .str() purge I might have to humbly step back.

[dexonsmith]

In D115421#3184249, @logan-5 wrote:

To be quite honest, in regards to the big .str() refactor (whether changing its type or deleting it), I feel I may have started to bite off more than I can chew here. There are at least a few thousand call sites of .str() throughout LLVM and its subprojects,

(FWIW, I bet most of those are unrelated to raw_string_ostream. Removing the function would probably find you a much smaller list.)

and as just a very occasional contributor who will be a distant stranger to most of them, I don't feel equipped or qualified to tackle auditing and fixing them myself. (Not to mention that it's orders of magnitude more work than I had planned to contribute with my initial patch.) I just wanted to optimize a few returns, and now we're talking about making a huge number of changes across the entire repo. I'm happy to bring this current patch through to landing, but when it comes to the big .str() purge I might have to humbly step back.

Fair enough! In that case, I suggest just removing the flush() from str() (as you have here) and adding a TODO to consider deleting it or making it return StringRef. Then at least anyone who looks can see it's obviously uninteresting; and you and/or others can incrementally delete the uninteresting calls over time.

Note also: git-grep gives me only 10 hits across llvm-project SetBuffered(); the only two outside of raw_ostream and its tests are llvm::errs().SetBuffered() in clangd. In case you're still worried about whether it's safe to do.

[logan-5]

Updated comment and added TODO.

[logan-5]

Thanks for the guidance, it's much appreciated.

In D115421#3184544, @dexonsmith wrote:

you and/or others can incrementally delete the uninteresting calls over time.

Doing it incrementally sounds much more approachable. I may indeed submit cleanup patches like this as time permits.

llvm/include/llvm/Support/raw_ostream.h

 };
 
 //===----------------------------------------------------------------------===//
 // Output Stream Adaptors
 //===----------------------------------------------------------------------===//
 
 /// 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;
 
   /// See raw_ostream::write_impl.
   void write_impl(const char *Ptr, size_t Size) override;
 
   /// Return the current position within the stream, not counting the bytes
   /// currently in the buffer.
   uint64_t current_pos() const override { return OS.size(); }
 
 public:
   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);
   }
 };
 
 /// A raw_ostream that writes to an SmallVector or SmallString.  This is a
 /// simple adaptor class. This class does not encounter output errors.

llvm/lib/Support/raw_ostream.cpp

 bool raw_fd_stream::classof(const raw_ostream *OS) {
   return OS->get_kind() == OStreamKind::OK_FDStream;
 }
 
 //===----------------------------------------------------------------------===//
 //  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);
 }
 
 //===----------------------------------------------------------------------===//
 //  raw_svector_ostream
 //===----------------------------------------------------------------------===//
 

llvm/unittests/Support/raw_ostream_test.cpp

 #include "llvm/Testing/Support/Error.h"
 #include "gtest/gtest.h"
 
 using namespace llvm;
 
 namespace {
 
 template<typename T> std::string printToString(const T &Value) {
   std::string res;
   {
     llvm::raw_string_ostream OS(res);
     OS.SetBuffered();
     OS << Value;
+    OS.flush();
   }
   return res;
 }
 
 /// printToString - Print the given value to a stream which only has \arg
 /// BytesLeftInBuffer bytes left in the buffer. This is useful for testing edge
 /// cases in the buffer handling logic.
 template<typename T> std::string printToString(const T &Value,
                                                unsigned BytesLeftInBuffer) {
   // FIXME: This is relying on internal knowledge of how raw_ostream works to

 
 TEST(raw_ostreamTest, TinyBuffer) {
   std::string Str;
   raw_string_ostream OS(Str);
   OS.SetBufferSize(1);
   OS << "hello";
   OS << 1;
   OS << 'w' << 'o' << 'r' << 'l' << 'd';
+  OS.flush();
   EXPECT_EQ("hello1world", OS.str());
 }
 
 TEST(raw_ostreamTest, WriteEscaped) {
   std::string Str;
 
   Str = "";
   raw_string_ostream(Str).write_escaped("hi");
   EXPECT_EQ("hi", Str);
TEST(raw_ostreamTest, flush_tied_to_stream_on_write) {
   TiedTo.tie(nullptr);
 
   // Calling tie with nullptr unties stream.
   TiedStream.SetUnbuffered();
   TiedStream.tie(nullptr);
   TiedTo << "y";
   TiedStream << "0";
   EXPECT_EQ("acegostuv", TiedToBuffer);
+
+  TiedTo.flush();
+  TiedStream.flush();
 }
 
 TEST(raw_ostreamTest, reserve_stream) {
   std::string Str;
   raw_string_ostream OS(Str);
   OS << "11111111111111111111";
   uint64_t CurrentPos = OS.tell();
   OS.reserveExtraSpace(1000);