pw_string: Add conversion warnings for all targets

This also adds assertion checks in cases where the length of the
incoming value might be greater than the maximum value of the string
size type.

Bug: b/259746255
Change-Id: I7af9a673cfd2db3d80e2380daf8f37f5850c70fc
Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/138331
Reviewed-by: Keir Mierle <keir@google.com>
Commit-Queue: Taylor Cramer <cramertj@google.com>

7 files changed, 80 insertions, 26 deletions

diff --git a/pw_string/BUILD.gn b/pw_string/BUILD.gn
index 9c7c06ac2..a2a9bc946 100644
--- a/pw_string/BUILD.gn
+++ b/pw_string/BUILD.gn
@@ -97,6 +97,9 @@ pw_source_set("string") {
     dir_pw_assert,
     dir_pw_polyfill,
   ]
+
+  # TODO(b/259746255): Remove this when everything compiles with -Wconversion.
+  configs = [ "$dir_pw_build:conversion_warnings" ]
 }
 
 pw_source_set("to_string") {
@@ -156,11 +159,17 @@ pw_test("string_test") {
   deps = [ ":string" ]
   sources = [ "string_test.cc" ]
   negative_compilation_tests = true
+
+  # TODO(b/259746255): Remove this when everything compiles with -Wconversion.
+  configs = [ "$dir_pw_build:conversion_warnings" ]
 }
 
 pw_test("string_builder_test") {
   deps = [ ":builder" ]
   sources = [ "string_builder_test.cc" ]
+
+  # TODO(b/259746255): Remove this when everything compiles with -Wconversion.
+  configs = [ "$dir_pw_build:conversion_warnings" ]
 }
 
 pw_test("to_string_test") {
@@ -169,16 +178,25 @@ pw_test("to_string_test") {
     ":pw_string",
   ]
   sources = [ "to_string_test.cc" ]
+
+  # TODO(b/259746255): Remove this when everything compiles with -Wconversion.
+  configs = [ "$dir_pw_build:conversion_warnings" ]
 }
 
 pw_test("type_to_string_test") {
   deps = [ ":to_string" ]
   sources = [ "type_to_string_test.cc" ]
+
+  # TODO(b/259746255): Remove this when everything compiles with -Wconversion.
+  configs = [ "$dir_pw_build:conversion_warnings" ]
 }
 
 pw_test("util_test") {
   deps = [ ":util" ]
   sources = [ "util_test.cc" ]
+
+  # TODO(b/259746255): Remove this when everything compiles with -Wconversion.
+  configs = [ "$dir_pw_build:conversion_warnings" ]
 }
 
 pw_doc_group("docs") {
diff --git a/pw_string/public/pw_string/internal/string_common_functions.inc b/pw_string/public/pw_string/internal/string_common_functions.inc
index c1c95cf11..c080bcca1 100644
--- a/pw_string/public/pw_string/internal/string_common_functions.inc
+++ b/pw_string/public/pw_string/internal/string_common_functions.inc
@@ -86,7 +86,8 @@ constexpr InlineBasicString& assign(InputIterator start, InputIterator finish) {
 }
 
 constexpr InlineBasicString& assign(std::initializer_list<T> list) {
-  return assign(list.begin(), list.size());
+  return assign(list.begin(),
+                static_cast<pw::string_impl::size_type>(list.size()));
 }
 
 #if PW_CXX_STANDARD_IS_SUPPORTED(17)  // std::string_view is a C++17 feature
@@ -96,7 +97,8 @@ template <typename StringView,
 constexpr InlineBasicString& assign(const StringView& string) {
   const std::basic_string_view<T> view = string;
   PW_ASSERT(view.size() < npos);
-  return assign(view.data(), view.size());
+  return assign(view.data(),
+                static_cast<pw::string_impl::size_type>(view.size()));
 }
 
 template <typename StringView,
@@ -106,8 +108,12 @@ constexpr InlineBasicString& assign(const StringView& string,
                                     size_type count = npos) {
   const std::basic_string_view<T> view = string;
   PW_ASSERT(view.size() < npos);
-  return static_cast<InlineBasicString&>(CopySubstr(
-      data(), view.data(), static_cast<size_type>(view.size()), index, count));
+  return static_cast<InlineBasicString&>(
+      CopySubstr(data(),
+                 view.data(),
+                 static_cast<pw::string_impl::size_type>(view.size()),
+                 index,
+                 count));
 }
 
 #endif  // PW_CXX_STANDARD_IS_SUPPORTED(17)
@@ -243,7 +249,7 @@ constexpr InlineBasicString& append(InputIterator first, InputIterator last) {
 }
 
 constexpr InlineBasicString& append(std::initializer_list<T> list) {
-  return append(list.begin(), list.size());
+  return append(list.begin(), pw::string_impl::CheckedCastToSize(list.size()));
 }
 
 #if PW_CXX_STANDARD_IS_SUPPORTED(17)  // std::string_view is a C++17 feature
@@ -253,7 +259,8 @@ template <typename StringView,
 constexpr InlineBasicString& append(const StringView& string) {
   const std::basic_string_view<T> view = string;
   PW_ASSERT(view.size() < npos);
-  return append(view.data(), view.size());
+  return append(view.data(),
+                static_cast<pw::string_impl::size_type>(view.size()));
 }
 
 template <typename StringView,
diff --git a/pw_string/public/pw_string/internal/string_impl.h b/pw_string/public/pw_string/internal/string_impl.h
index 5c2eb0de1..1c56aeb59 100644
--- a/pw_string/public/pw_string/internal/string_impl.h
+++ b/pw_string/public/pw_string/internal/string_impl.h
@@ -13,6 +13,7 @@
 // the License.
 #pragma once
 
+#include <limits>
 #include <string>  // for std::char_traits
 #include <type_traits>
 
@@ -104,6 +105,16 @@ constexpr bool NullTerminatedArrayFitsInString(
          null_terminated_array_size - 1 < kGeneric;
 }
 
+// Used to safely convert various numeric types to `size_type`.
+template <typename T>
+constexpr size_type CheckedCastToSize(T num) {
+  static_assert(std::is_unsigned<T>::value,
+                "Attempted to convert signed value to string length, but only "
+                "unsigned types are allowed.");
+  PW_ASSERT(num < std::numeric_limits<size_type>::max());
+  return static_cast<size_type>(num);
+}
+
 // Constexpr utility functions for pw::InlineString. These are NOT intended for
 // general use. These mostly map directly to general purpose standard library
 // utilities that are not constexpr until C++20.
diff --git a/pw_string/public/pw_string/string.h b/pw_string/public/pw_string/string.h
index a6041f52a..01477dcdd 100644
--- a/pw_string/public/pw_string/string.h
+++ b/pw_string/public/pw_string/string.h
@@ -137,7 +137,9 @@ class InlineBasicString final
   }
 
   constexpr InlineBasicString(std::initializer_list<T> list)
-      : InlineBasicString(list.begin(), list.size()) {}
+      : InlineBasicString(
+            list.begin(),
+            static_cast<pw::string_impl::size_type>(list.size())) {}
 
 #if PW_CXX_STANDARD_IS_SUPPORTED(17)  // std::string_view is a C++17 feature
   // Unlike std::string, pw::InlineString<> supports implicit conversions from
@@ -164,7 +166,8 @@ class InlineBasicString final
                 std::is_same<StringView, std::basic_string_view<T>>::value>* =
                 nullptr>
   constexpr InlineBasicString(const StringView& view)
-      : InlineBasicString(view.data(), view.size()) {}
+      : InlineBasicString(view.data(),
+                          pw::string_impl::CheckedCastToSize(view.size())) {}
 
   template <typename StringView,
             typename = string_impl::EnableIfStringViewLike<T, StringView>>
@@ -173,7 +176,11 @@ class InlineBasicString final
                               size_type count)
       : InlineBasicString() {
     const std::basic_string_view<T> view = string;
-    CopySubstr(data(), view.data(), view.size(), index, count);
+    CopySubstr(data(),
+               view.data(),
+               pw::string_impl::CheckedCastToSize(view.size()),
+               index,
+               count);
   }
 #endif  // PW_CXX_STANDARD_IS_SUPPORTED(17)
 
@@ -246,7 +253,8 @@ class InlineBasicString final
   }
 
   constexpr InlineBasicString& operator+=(std::initializer_list<T> list) {
-    return append(list.begin(), list.size());
+    return append(list.begin(),
+                  static_cast<pw::string_impl::size_type>(list.size()));
   }
 
 #if PW_CXX_STANDARD_IS_SUPPORTED(17)  // std::string_view is a C++17 feature
diff --git a/pw_string/string_builder_test.cc b/pw_string/string_builder_test.cc
index ad05c8a15..5252dc6b9 100644
--- a/pw_string/string_builder_test.cc
+++ b/pw_string/string_builder_test.cc
@@ -192,26 +192,30 @@ TEST(StringBuilder, Append_NonTerminatedString) {
 TEST(StringBuilder, Append_Chars) {
   StringBuffer<8> sb;
 
-  EXPECT_TRUE(sb.append(7, '?').ok());
+  size_t count = 7;
+  EXPECT_TRUE(sb.append(count, '?').ok());
   EXPECT_STREQ("???????", sb.data());
 }
 
 TEST(StringBuilder, Append_Chars_Full) {
   StringBuffer<8> sb;
 
-  EXPECT_EQ(Status::ResourceExhausted(), sb.append(8, '?').last_status());
+  size_t count = 8;
+  EXPECT_EQ(Status::ResourceExhausted(), sb.append(count, '?').last_status());
   EXPECT_STREQ("???????", sb.data());
 }
 
 TEST(StringBuilder, Append_Chars_ToEmpty) {
   StringBuilder sb(span<char>{});
 
-  EXPECT_EQ(Status::ResourceExhausted(), sb.append(1, '?').last_status());
+  size_t count = 1;
+  EXPECT_EQ(Status::ResourceExhausted(), sb.append(count, '?').last_status());
 }
 
 TEST(StringBuilder, Append_PartialCString) {
   StringBuffer<12> sb;
-  EXPECT_TRUE(sb.append("123456", 4).ok());
+  size_t count = 4;
+  EXPECT_TRUE(sb.append("123456", count).ok());
   EXPECT_EQ(4u, sb.size());
   EXPECT_STREQ("1234", sb.data());
 }
@@ -225,7 +229,9 @@ TEST(StringBuilder, Append_CString) {
 
 TEST(StringBuilder, Append_CString_Full) {
   auto sb = MakeString<6>("hello");
-  EXPECT_EQ(Status::ResourceExhausted(), sb.append("890123", 1).last_status());
+  size_t count = 1;
+  EXPECT_EQ(Status::ResourceExhausted(),
+            sb.append("890123", count).last_status());
   EXPECT_EQ(Status::ResourceExhausted(), sb.status());
   EXPECT_EQ(sb.max_size(), sb.size());
   EXPECT_STREQ("hello", sb.data());
@@ -239,13 +245,16 @@ TEST(StringBuilder, Append_StringView) {
 
 TEST(StringBuilder, Append_StringView_Substring) {
   auto sb = MakeString<32>("I like ");
-  EXPECT_TRUE(sb.append("your shoes!!!"sv, 5, 5).ok());
+  size_t position = 5;
+  size_t count = 5;
+  EXPECT_TRUE(sb.append("your shoes!!!"sv, position, count).ok());
   EXPECT_EQ("I like shoes"sv, sb);
 }
 
 TEST(StringBuilder, Append_StringView_RemainingSubstring) {
   auto sb = MakeString<32>("I like ");
-  EXPECT_TRUE(sb.append("your shoes!!!"sv, 5).ok());
+  size_t count = 5;
+  EXPECT_TRUE(sb.append("your shoes!!!"sv, count).ok());
   EXPECT_EQ("I like shoes!!!"sv, sb);
 }
 
diff --git a/pw_string/to_string_test.cc b/pw_string/to_string_test.cc
index 39a09fbdd..8ec9c498d 100644
--- a/pw_string/to_string_test.cc
+++ b/pw_string/to_string_test.cc
@@ -123,7 +123,7 @@ TEST(ToString, Float) {
   if (string::internal::config::kEnableDecimalFloatExpansion) {
     EXPECT_EQ(5u, ToString(0.0f, buffer).size());
     EXPECT_STREQ("0.000", buffer);
-    EXPECT_EQ(6u, ToString(33.444, buffer).size());
+    EXPECT_EQ(6u, ToString(33.444f, buffer).size());
     EXPECT_STREQ("33.444", buffer);
     EXPECT_EQ(3u, ToString(INFINITY, buffer).size());
     EXPECT_STREQ("inf", buffer);
@@ -141,10 +141,11 @@ TEST(ToString, Float) {
 
 TEST(ToString, Pointer_NonNull_WritesValue) {
   CustomType custom;
-  const size_t length = std::snprintf(expected,
-                                      sizeof(expected),
-                                      "%" PRIxPTR,
-                                      reinterpret_cast<intptr_t>(&custom));
+  const size_t length =
+      static_cast<size_t>(std::snprintf(expected,
+                                        sizeof(expected),
+                                        "%" PRIxPTR,
+                                        reinterpret_cast<intptr_t>(&custom)));
 
   EXPECT_EQ(length, ToString(&custom, buffer).size());
   EXPECT_STREQ(expected, buffer);
diff --git a/pw_string/type_to_string_test.cc b/pw_string/type_to_string_test.cc
index f9fe670a3..2b2e8fe9f 100644
--- a/pw_string/type_to_string_test.cc
+++ b/pw_string/type_to_string_test.cc
@@ -346,12 +346,12 @@ TEST_F(FloatAsIntToStringTest, NegativeNan) {
 }
 
 TEST_F(FloatAsIntToStringTest, RoundDown_PrintsNearestInt) {
-  EXPECT_EQ(1u, FloatAsIntToString(1.23, buffer_).size());
+  EXPECT_EQ(1u, FloatAsIntToString(1.23f, buffer_).size());
   EXPECT_STREQ("1", buffer_);
 }
 
 TEST_F(FloatAsIntToStringTest, RoundUp_PrintsNearestInt) {
-  EXPECT_EQ(4u, FloatAsIntToString(1234.5, buffer_).size());
+  EXPECT_EQ(4u, FloatAsIntToString(1234.5f, buffer_).size());
   EXPECT_STREQ("1235", buffer_);
 }
 
@@ -366,13 +366,13 @@ TEST_F(FloatAsIntToStringTest, RoundsToPositiveZero_PrintsZero) {
 }
 
 TEST_F(FloatAsIntToStringTest, RoundDownNegative_PrintsNearestInt) {
-  volatile float x = -5.9;
+  volatile float x = -5.9f;
   EXPECT_EQ(2u, FloatAsIntToString(x, buffer_).size());
   EXPECT_STREQ("-6", buffer_);
 }
 
 TEST_F(FloatAsIntToStringTest, RoundUpNegative_PrintsNearestInt) {
-  EXPECT_EQ(9u, FloatAsIntToString(-50000000.1, buffer_).size());
+  EXPECT_EQ(9u, FloatAsIntToString(-50000000.1f, buffer_).size());
   EXPECT_STREQ("-50000000", buffer_);
 }