Make SimpleStringBuilder into a non-template

So that future CLs can de-inline its methods.

We do this by asking the caller to allocate the buffer instead of
having it as a data member.

Bug: webrtc:8982
Change-Id: I246b0973e54510fdd880c3b6875336c31334d008
Reviewed-on: https://webrtc-review.googlesource.com/60000
Commit-Queue: Karl Wiberg <kwiberg@webrtc.org>
Reviewed-by: Fredrik Solenberg <solenberg@webrtc.org>
Reviewed-by: Tommi <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#22355}

audio/audio_receive_stream.cc

@@ -30,7 +30,8 @@
 namespace webrtc {
 
 std::string AudioReceiveStream::Config::Rtp::ToString() const {
-  rtc::SimpleStringBuilder<1024> ss;
+  char ss_buf[1024];
+  rtc::SimpleStringBuilder ss(ss_buf);
   ss << "{remote_ssrc: " << remote_ssrc;
   ss << ", local_ssrc: " << local_ssrc;
   ss << ", transport_cc: " << (transport_cc ? "on" : "off");
@@ -48,7 +49,8 @@ std::string AudioReceiveStream::Config::Rtp::ToString() const {
 }
 
 std::string AudioReceiveStream::Config::ToString() const {
-  rtc::SimpleStringBuilder<1024> ss;
+  char ss_buf[1024];
+  rtc::SimpleStringBuilder ss(ss_buf);
   ss << "{rtp: " << rtp.ToString();
   ss << ", rtcp_send_transport: "
      << (rtcp_send_transport ? "(Transport)" : "null");

rtc_base/BUILD.gn

@@ -142,12 +142,15 @@ rtc_source_set("safe_minmax") {
 
 rtc_source_set("stringutils") {
   sources = [
+    "strings/string_builder.cc",
     "strings/string_builder.h",
     "stringutils.cc",
     "stringutils.h",
   ]
   deps = [
     ":checks",
+    ":safe_minmax",
+    "../api:array_view",
   ]
 }
 

rtc_base/logging.cc

@@ -127,7 +127,8 @@ LogMessage::LogMessage(const char* file,
     print_stream_ << "(" << FilenameFromPath(file)  << ":" << line << "): ";
 
   if (err_ctx != ERRCTX_NONE) {
-    SimpleStringBuilder<1024> tmp;
+    char tmp_buf[1024];
+    SimpleStringBuilder tmp(tmp_buf);
     tmp.AppendFormat("[0x%08X]", err);
     switch (err_ctx) {
       case ERRCTX_ERRNO:

rtc_base/strings/string_builder.cc

@@ -0,0 +1,21 @@
+/*
+ *  Copyright 2018 The WebRTC Project Authors. All rights reserved.
+ *
+ *  Use of this source code is governed by a BSD-style license
+ *  that can be found in the LICENSE file in the root of the source
+ *  tree. An additional intellectual property rights grant can be found
+ *  in the file PATENTS.  All contributing project authors may
+ *  be found in the AUTHORS file in the root of the source tree.
+ */
+
+#include "rtc_base/strings/string_builder.h"
+
+namespace rtc {
+
+SimpleStringBuilder::SimpleStringBuilder(rtc::ArrayView<char> buffer)
+    : buffer_(buffer) {
+  buffer_[0] = '\0';
+  RTC_DCHECK(IsConsistent());
+}
+
+}  // namespace rtc

rtc_base/strings/string_builder.h

@@ -12,22 +12,24 @@
 #define RTC_BASE_STRINGS_STRING_BUILDER_H_
 
 #include <cstdio>
+#include <cstring>
 #include <string>
 
+#include "api/array_view.h"
 #include "rtc_base/checks.h"
+#include "rtc_base/numerics/safe_minmax.h"
 #include "rtc_base/stringutils.h"
 
 namespace rtc {
 
-// This is a minimalistic string builder class meant to cover the most cases
+// This is a minimalistic string builder class meant to cover the most cases of
-// of when you might otherwise be tempted to use a stringstream (discouraged
+// when you might otherwise be tempted to use a stringstream (discouraged for
-// for anything except logging).
+// anything except logging). It uses a fixed-size buffer provided by the caller
-// This class allocates a fixed size buffer on the stack and concatenates
+// and concatenates strings and numbers into it, allowing the results to be
-// strings and numbers into it, allowing the results to be read via |str()|.
+// read via |str()|.
-template <size_t buffer_size>
 class SimpleStringBuilder {
  public:
-  SimpleStringBuilder() { buffer_[0] = '\0'; }
+  explicit SimpleStringBuilder(rtc::ArrayView<char> buffer);
   SimpleStringBuilder(const SimpleStringBuilder&) = delete;
   SimpleStringBuilder& operator=(const SimpleStringBuilder&) = delete;
 
@@ -80,48 +82,60 @@ class SimpleStringBuilder {
   // Returns a pointer to the built string. The name |str()| is borrowed for
   // compatibility reasons as we replace usage of stringstream throughout the
   // code base.
-  const char* str() const { return &buffer_[0]; }
+  const char* str() const { return buffer_.data(); }
 
   // Returns the length of the string. The name |size()| is picked for STL
   // compatibility reasons.
   size_t size() const { return size_; }
 
   // Allows appending a printf style formatted string.
-  SimpleStringBuilder& AppendFormat(const char* fmt, ...) {
+#if defined(__GNUC__)
+  __attribute__((__format__(__printf__, 2, 3)))
+#endif
+  SimpleStringBuilder&
+  AppendFormat(const char* fmt, ...) {
     va_list args;
     va_start(args, fmt);
-    int len = std::vsnprintf(&buffer_[size_], buffer_size - size_, fmt, args);
+    const int len =
-    RTC_DCHECK_GE(len, 0);
+        std::vsnprintf(&buffer_[size_], buffer_.size() - size_, fmt, args);
-    // Negative values are likely programmer error, but let's not update the
+    if (len >= 0) {
-    // length if so.
+      const size_t chars_added = rtc::SafeMin(len, buffer_.size() - 1 - size_);
-    if (len > 0)
+      size_ += chars_added;
-      AddToLength(len);
+      RTC_DCHECK_EQ(len, chars_added) << "Buffer size was insufficient";
+    } else {
+      // This should never happen, but we're paranoid, so re-write the
+      // terminator in case vsnprintf() overwrote it.
+      RTC_NOTREACHED();
+      buffer_[size_] = '\0';
+    }
     va_end(args);
+    RTC_DCHECK(IsConsistent());
     return *this;
   }
 
   // An alternate way from operator<<() to append a string. This variant is
   // slightly more efficient when the length of the string to append, is known.
   SimpleStringBuilder& Append(const char* str, size_t length = SIZE_UNKNOWN) {
-    AddToLength(
+    const size_t chars_added =
-        rtc::strcpyn(&buffer_[size_], buffer_size - size_, str, length));
+        rtc::strcpyn(&buffer_[size_], buffer_.size() - size_, str, length);
+    size_ += chars_added;
+    RTC_DCHECK_EQ(chars_added,
+                  length == SIZE_UNKNOWN ? std::strlen(str) : length)
+        << "Buffer size was insufficient";
+    RTC_DCHECK(IsConsistent());
     return *this;
   }
 
  private:
-  void AddToLength(size_t chars_added) {
+  bool IsConsistent() const {
-    size_ += chars_added;
+    return size_ <= buffer_.size() - 1 && buffer_[size_] == '\0';
-    RTC_DCHECK_EQ('\0', buffer_[size_]);
-    RTC_DCHECK_LE(size_, buffer_size - 1)
-        << "Buffer size limit reached (" << buffer_size << ")";
   }
 
-  // An always-zero-terminated fixed buffer that we write to.
+  // An always-zero-terminated fixed-size buffer that we write to. The fixed
-  // Assuming the SimpleStringBuilder instance lives on the stack, this
+  // size allows the buffer to be stack allocated, which helps performance.
-  // buffer will be stack allocated, which is done for performance reasons.
   // Having a fixed size is furthermore useful to avoid unnecessary resizing
   // while building it.
-  char buffer_[buffer_size];  // NOLINT
+  const rtc::ArrayView<char> buffer_;
 
   // Represents the number of characters written to the buffer.
   // This does not include the terminating '\0'.

rtc_base/strings/string_builder_unittest.cc

@@ -11,13 +11,15 @@
 #include "rtc_base/strings/string_builder.h"
 
 #include "rtc_base/checks.h"
-#include "rtc_base/gunit.h"
 #include "rtc_base/stringutils.h"
+#include "test/gmock.h"
+#include "test/gtest.h"
 
 namespace rtc {
 
 TEST(SimpleStringBuilder, Limit) {
-  SimpleStringBuilder<10> sb;
+  char sb_buf[10];
+  SimpleStringBuilder sb(sb_buf);
   EXPECT_EQ(0u, strlen(sb.str()));
 
   // Test that for a SSB with a buffer size of 10, that we can write 9 chars
@@ -27,26 +29,115 @@ TEST(SimpleStringBuilder, Limit) {
 }
 
 TEST(SimpleStringBuilder, NumbersAndChars) {
-  SimpleStringBuilder<100> sb;
+  char sb_buf[100];
+  SimpleStringBuilder sb(sb_buf);
   sb << 1 << ':' << 2.1 << ":" << 2.2f << ':' << 78187493520ll << ':'
      << 78187493520ul;
   EXPECT_EQ(0, strcmp(sb.str(), "1:2.100000:2.200000:78187493520:78187493520"));
 }
 
 TEST(SimpleStringBuilder, Format) {
-  SimpleStringBuilder<100> sb;
+  char sb_buf[100];
+  SimpleStringBuilder sb(sb_buf);
   sb << "Here we go - ";
-  sb.AppendFormat("This is a hex formatted value: 0x%08x", 3735928559);
+  sb.AppendFormat("This is a hex formatted value: 0x%08llx", 3735928559ULL);
   EXPECT_EQ(0,
             strcmp(sb.str(),
                    "Here we go - This is a hex formatted value: 0xdeadbeef"));
 }
 
 TEST(SimpleStringBuilder, StdString) {
-  SimpleStringBuilder<100> sb;
+  char sb_buf[100];
+  SimpleStringBuilder sb(sb_buf);
   std::string str = "does this work?";
   sb << str;
   EXPECT_EQ(str, sb.str());
 }
 
+// These tests are safe to run if we have death test support or if DCHECKs are
+// off.
+#if (GTEST_HAS_DEATH_TEST && !defined(WEBRTC_ANDROID)) || !RTC_DCHECK_IS_ON
+
+TEST(SimpleStringBuilder, BufferOverrunConstCharP) {
+  char sb_buf[4];
+  SimpleStringBuilder sb(sb_buf);
+  const char* const msg = "This is just too much";
+#if RTC_DCHECK_IS_ON
+  EXPECT_DEATH(sb << msg, "");
+#else
+  sb << msg;
+  EXPECT_THAT(sb.str(), testing::StrEq("Thi"));
+#endif
+}
+
+TEST(SimpleStringBuilder, BufferOverrunStdString) {
+  char sb_buf[4];
+  SimpleStringBuilder sb(sb_buf);
+  sb << 12;
+  const std::string msg = "Aw, come on!";
+#if RTC_DCHECK_IS_ON
+  EXPECT_DEATH(sb << msg, "");
+#else
+  sb << msg;
+  EXPECT_THAT(sb.str(), testing::StrEq("12A"));
+#endif
+}
+
+TEST(SimpleStringBuilder, BufferOverrunInt) {
+  char sb_buf[4];
+  SimpleStringBuilder sb(sb_buf);
+  constexpr int num = -12345;
+#if RTC_DCHECK_IS_ON
+  EXPECT_DEATH(sb << num, "");
+#else
+  sb << num;
+  // If we run into the end of the buffer, resonable results are either that
+  // the append has no effect or that it's truncated at the point where the
+  // buffer ends.
+  EXPECT_THAT(sb.str(),
+              testing::AnyOf(testing::StrEq(""), testing::StrEq("-12")));
+#endif
+}
+
+TEST(SimpleStringBuilder, BufferOverrunDouble) {
+  char sb_buf[5];
+  SimpleStringBuilder sb(sb_buf);
+  constexpr double num = 123.456;
+#if RTC_DCHECK_IS_ON
+  EXPECT_DEATH(sb << num, "");
+#else
+  sb << num;
+  EXPECT_THAT(sb.str(),
+              testing::AnyOf(testing::StrEq(""), testing::StrEq("123.")));
+#endif
+}
+
+TEST(SimpleStringBuilder, BufferOverrunConstCharPAlreadyFull) {
+  char sb_buf[4];
+  SimpleStringBuilder sb(sb_buf);
+  sb << 123;
+  const char* const msg = "This is just too much";
+#if RTC_DCHECK_IS_ON
+  EXPECT_DEATH(sb << msg, "");
+#else
+  sb << msg;
+  EXPECT_THAT(sb.str(), testing::StrEq("123"));
+#endif
+}
+
+TEST(SimpleStringBuilder, BufferOverrunIntAlreadyFull) {
+  char sb_buf[4];
+  SimpleStringBuilder sb(sb_buf);
+  sb << "xyz";
+  constexpr int num = -12345;
+#if RTC_DCHECK_IS_ON
+  EXPECT_DEATH(sb << num, "");
+#else
+  sb << num;
+  EXPECT_THAT(sb.str(), testing::StrEq("xyz"));
+#endif
+}
+
+#endif
+
 }  // namespace rtc

video/receive_statistics_proxy.cc

@@ -64,7 +64,8 @@ const char* UmaPrefixForContentType(VideoContentType content_type) {
 }
 
 std::string UmaSuffixForContentType(VideoContentType content_type) {
-  rtc::SimpleStringBuilder<1024> ss;
+  char ss_buf[1024];
+  rtc::SimpleStringBuilder ss(ss_buf);
   int simulcast_id = videocontenttypehelpers::GetSimulcastId(content_type);
   if (simulcast_id > 0) {
     ss << ".S" << simulcast_id - 1;
@@ -135,7 +136,8 @@ ReceiveStatisticsProxy::~ReceiveStatisticsProxy() {
 
 void ReceiveStatisticsProxy::UpdateHistograms() {
   RTC_DCHECK_RUN_ON(&decode_thread_);
-  rtc::SimpleStringBuilder<8 * 1024> log_stream;
+  char log_stream_buf[8 * 1024];
+  rtc::SimpleStringBuilder log_stream(log_stream_buf);
   int stream_duration_sec = (clock_->TimeInMilliseconds() - start_ms_) / 1000;
   if (stats_.frame_counts.key_frames > 0 ||
       stats_.frame_counts.delta_frames > 0) {

video/send_statistics_proxy.cc

@@ -268,7 +268,8 @@ void SendStatisticsProxy::UmaSamplesContainer::UpdateHistograms(
   RTC_DCHECK(uma_prefix_ == kRealtimePrefix || uma_prefix_ == kScreenPrefix);
   const int kIndex = uma_prefix_ == kScreenPrefix ? 1 : 0;
   const int kMinRequiredPeriodicSamples = 6;
-  rtc::SimpleStringBuilder<8 * 1024> log_stream;
+  char log_stream_buf[8 * 1024];
+  rtc::SimpleStringBuilder log_stream(log_stream_buf);
   int in_width = input_width_counter_.Avg(kMinRequiredMetricsSamples);
   int in_height = input_height_counter_.Avg(kMinRequiredMetricsSamples);
   if (in_width != -1) {