[llvm-branch-commits] Address review feedback (PR #113367)

[via llvm-branch-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-llvm-support

Author: Steven Wu (cachemeifyoucan)

<details>
<summary>Changes</summary>



---
Full diff: https://github.com/llvm/llvm-project/pull/113367.diff


2 Files Affected:

- (modified) llvm/include/llvm/Support/raw_ostream_proxy.h (+23-9) 
- (modified) llvm/unittests/Support/raw_ostream_proxy_test.cpp (+3-3) 


``````````diff
diff --git a/llvm/include/llvm/Support/raw_ostream_proxy.h b/llvm/include/llvm/Support/raw_ostream_proxy.h
index 093d0a927833f1..d661ccbbbd616c 100644
--- a/llvm/include/llvm/Support/raw_ostream_proxy.h
+++ b/llvm/include/llvm/Support/raw_ostream_proxy.h
@@ -17,7 +17,6 @@ namespace llvm {
 /// template instantions.
 class raw_ostream_proxy_adaptor_base {
 protected:
-  raw_ostream_proxy_adaptor_base() = delete;
   raw_ostream_proxy_adaptor_base(const raw_ostream_proxy_adaptor_base &) =
       delete;
 
@@ -72,8 +71,7 @@ class raw_ostream_proxy_adaptor_base {
 /// changeColor(), resetColor(), and \a reverseColor() are not forwarded, since
 /// they need to call \a flush() and the buffer lives in the proxy.
 template <class RawOstreamT = raw_ostream>
-class raw_ostream_proxy_adaptor : public RawOstreamT,
-                                  public raw_ostream_proxy_adaptor_base {
+class raw_ostream_proxy_adaptor : public RawOstreamT {
   void write_impl(const char *Ptr, size_t Size) override {
     getProxiedOS().write(Ptr, Size);
   }
@@ -92,23 +90,39 @@ class raw_ostream_proxy_adaptor : public RawOstreamT,
     RawOstreamT::enable_colors(enable);
     getProxiedOS().enable_colors(enable);
   }
+  bool hasProxiedOS() const { return OS; }
+  raw_ostream &getProxiedOS() const {
+    assert(OS && "raw_ostream_proxy_adaptor use after reset");
+    return *OS;
+  }
+  size_t getPreferredBufferSize() const { return PreferredBufferSize; }
 
   ~raw_ostream_proxy_adaptor() override { resetProxiedOS(); }
 
 protected:
   template <class... ArgsT>
   explicit raw_ostream_proxy_adaptor(raw_ostream &OS, ArgsT &&...Args)
-      : RawOstreamT(std::forward<ArgsT>(Args)...),
-        raw_ostream_proxy_adaptor_base(OS) {}
+      : RawOstreamT(std::forward<ArgsT>(Args)...), OS(&OS),
+        PreferredBufferSize(OS.GetBufferSize()) {
+    // Drop OS's buffer to make this->flush() forward. This proxy will add a
+    // buffer in its place.
+    OS.SetUnbuffered();
+  }
 
   /// Stop proxying the stream. Flush and set up a crash for future writes.
   ///
   /// For example, this can simplify logic when a subclass might have a longer
   /// lifetime than the stream it proxies.
   void resetProxiedOS() {
-    raw_ostream_proxy_adaptor_base::resetProxiedOS(*this);
+    OS->SetUnbuffered();
+    OS = nullptr;
   }
-  void resetProxiedOS(raw_ostream &) = delete;
+
+private:
+  raw_ostream *OS;
+
+  /// Caches the value of OS->GetBufferSize() at construction time.
+  size_t PreferredBufferSize;
 };
 
 /// Adaptor for creating a stream that proxies a \a raw_pwrite_stream.
@@ -134,7 +148,7 @@ class raw_pwrite_stream_proxy_adaptor
   }
 };
 
-/// Non-owning proxy for a \a raw_ostream. Enables passing a stream into of an
+/// Non-owning proxy for a \a raw_ostream. Enables passing a stream into an
 /// API that takes ownership.
 class raw_ostream_proxy : public raw_ostream_proxy_adaptor<> {
   void anchor() override;
@@ -144,7 +158,7 @@ class raw_ostream_proxy : public raw_ostream_proxy_adaptor<> {
 };
 
 /// Non-owning proxy for a \a raw_pwrite_stream. Enables passing a stream
-/// into of an API that takes ownership.
+/// into an API that takes ownership.
 class raw_pwrite_stream_proxy : public raw_pwrite_stream_proxy_adaptor<> {
   void anchor() override;
 
diff --git a/llvm/unittests/Support/raw_ostream_proxy_test.cpp b/llvm/unittests/Support/raw_ostream_proxy_test.cpp
index ee97fe65b66003..1233ed54fd5a52 100644
--- a/llvm/unittests/Support/raw_ostream_proxy_test.cpp
+++ b/llvm/unittests/Support/raw_ostream_proxy_test.cpp
@@ -22,7 +22,7 @@ class BufferedNoPwriteSmallVectorStream : public raw_ostream {
 public:
   // Choose a strange buffer size to ensure it doesn't collide with the default
   // on \a raw_ostream.
-  constexpr static const size_t PreferredBufferSize = 63;
+  static constexpr size_t PreferredBufferSize = 63;
 
   size_t preferred_buffer_size() const override { return PreferredBufferSize; }
   uint64_t current_pos() const override { return Vector.size(); }
@@ -40,7 +40,7 @@ class BufferedNoPwriteSmallVectorStream : public raw_ostream {
   bool IsDisplayed = false;
 };
 
-constexpr const size_t BufferedNoPwriteSmallVectorStream::PreferredBufferSize;
+constexpr size_t BufferedNoPwriteSmallVectorStream::PreferredBufferSize;
 
 TEST(raw_ostream_proxyTest, write) {
   // Besides confirming that "write" works, this test confirms that the proxy
@@ -157,7 +157,7 @@ TEST(raw_ostream_proxyTest, resetProxiedOS) {
   EXPECT_EQ(0u, ProxyOS.GetBufferSize());
   EXPECT_FALSE(ProxyOS.hasProxiedOS());
 
-#if GTEST_HAS_DEATH_TEST
+#if GTEST_HAS_DEATH_TEST && !defined(NDEBUG)
   EXPECT_DEATH(ProxyOS << "e", "use after reset");
   EXPECT_DEATH(ProxyOS.getProxiedOS(), "use after reset");
 #endif

``````````

</details>


https://github.com/llvm/llvm-project/pull/113367