pw_{containers,string}: Remove string features from pw::Vector<char>

- Delete pw::string::Copy() overloads for using pw::Vector<char> for
  strings.
- Remove the pw::Vector<char>::view() function.
- Prevent use of pw::InlineString<> as the destination for
  pw::string::Copy(). Should use pw::InlineString<>'s assign() functions
  or pw::string::Assign() instead.

Change-Id: I63aab601f6acc83d244e67a4fdd3b38b2004fd1b
Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/110112
Pigweed-Auto-Submit: Wyatt Hepler <hepler@google.com>
Reviewed-by: Armando Montanez <amontanez@google.com>
Reviewed-by: Scott James Remnant <keybuk@google.com>
Commit-Queue: Wyatt Hepler <hepler@google.com>

pw_containers/docs.rst

@@ -18,61 +18,6 @@ size in a variable. This allows Vectors to be used without having to know
 their maximum size at compile time. It also keeps code size small since
 function implementations are shared for all maximum sizes.
 
-pw::Vector<char>
-----------------
-As a variable length type with a fixed-size buffer, ``Vector<char>`` makes a
-useful lightweight container for strings, avoiding the computation and bug-prone
-nature of null-termination, and overhead of ``pw::StringBuffer`` for long-term
-storage.
-
-To facilitate this use, ``Vector<char>`` may be initialized from a
-``const char*`` or  ``std::string_view``, and provides a ``view()`` method
-and conversion operator to obtain a ``std::string_view`` over the character
-data.
-
-.. code:: c++
-
-  // Initialize from string.
-  pw::Vector<char, 32> greeting = "Hello";
-
-.. code:: c++
-
-  // Initialize as part of an aggregate
-  struct Person {
-    pw::Vector<char, 32> name;
-    int age;
-  };
-
-  Person bob = {
-    .name = "Bob",
-    .age = 42;
-  };
-
-.. code:: c++
-
-  // Initialize using std::string_view conversion from pw::StringBuffer
-  pw::StringBuffer<150> buffer;
-  buffer << "Hello";
-  pw::Vector<char, 32> greeting = buffer;
-
-  // Or use conversion to std::string_view to add to a buffer.
-  buffer << " " << person.name;
-
-Since the internal array is not null-terminated, to use the string in code that
-expects that, :ref:`module-pw_string` provides a version of ``pw::string::Copy``
-that takes a vector as a source.
-
-.. code:: c++
-
-  #include "pw_containers/vector.h"
-  #include "pw_string/vector.h"
-
-  pw::Vector<char, 32> greeting = "Hello";
-  char c_str[33];
-
-  pw::string::Copy(greeting, c_str);
-  printf("%s\n", greeting);
-
 pw::IntrusiveList
 =================
 IntrusiveList provides an embedded-friendly singly-linked intrusive list

pw_containers/public/pw_containers/vector.h

@@ -282,22 +282,6 @@ class Vector<T, vector_impl::kGeneric>
     return static_cast<const Vector<T, 0>*>(this)->array();
   }
 
-  // Returns a std::string_view of the contents of this Vector when T is char.
-  // The std::string_view is invalidated if the vector contents change.
-  template <typename U = T,
-            typename = std::enable_if_t<std::is_same_v<U, char>>>
-  constexpr std::string_view view() const {
-    return std::string_view(data(), size());
-  }
-
-  // Allow implicit conversions to std::string_view when T is char so Vectors
-  // can be passed into functions that take a std::string_view.
-  template <typename U = T,
-            typename = std::enable_if_t<std::is_same_v<U, char>>>
-  constexpr operator std::string_view() const {
-    return view();
-  }
-
   // Iterate
 
   iterator begin() noexcept { return &data()[0]; }

pw_containers/vector_test.cc

@@ -488,17 +488,6 @@ TEST(Vector, Generic) {
   }
 }
 
-TEST(Vector, StringView) {
-  Vector<char, 8> vector{"Hello"sv};
-  EXPECT_EQ(vector.view().size(), vector.size());
-  EXPECT_EQ(vector.view().data(), vector.data());
-}
-
-TEST(Vector, StringViewConversion) {
-  Vector<char, 8> vector{"Hello"sv};
-  EXPECT_EQ(vector, "Hello"sv);
-}
-
 // Test that Vector<T> is trivially destructible when its type is.
 static_assert(std::is_trivially_destructible_v<Vector<int>>);
 static_assert(std::is_trivially_destructible_v<Vector<int, 4>>);

pw_protobuf/BUILD.gn

@@ -156,7 +156,6 @@ pw_test("codegen_encoder_test") {
 pw_test("codegen_message_test") {
   deps = [
     ":codegen_test_protos.pwpb",
-    "$dir_pw_string:vector",
     dir_pw_string,
   ]
   sources = [ "codegen_message_test.cc" ]

pw_protobuf/codegen_message_test.cc

@@ -23,7 +23,6 @@
 #include "pw_status/status.h"
 #include "pw_status/status_with_size.h"
 #include "pw_stream/memory_stream.h"
-#include "pw_string/vector.h"
 
 // These header files contain the code generated by the pw_protobuf plugin.
 // They are re-generated every time the tests are built and are used by the
@@ -1295,14 +1294,14 @@ TEST(CodegenMessage, Write) {
   message.ziggy = -111;
   message.cycles = 0x40302010fecaaddeu;
   message.ratio = -1.42f;
-  pw::string::Copy("not a typewriter", message.error_message).IgnoreError();
+  message.error_message = "not a typewriter";
   message.pigweed.status = Bool::FILE_NOT_FOUND;
   message.bin = Pigweed::Protobuf::Binary::ZERO;
   message.bungle = -111;
   message.proto.bin = Proto::Binary::OFF;
   message.proto.pigweed_pigweed_bin = Pigweed::Pigweed::Binary::ZERO;
   message.proto.pigweed_protobuf_bin = Pigweed::Protobuf::Binary::ZERO;
-  pw::string::Copy("/etc/passwd", message.proto.meta.file_name).IgnoreError();
+  message.proto.meta.file_name = "/etc/passwd";
   message.proto.meta.status = Pigweed::Protobuf::Compiler::Status::FUBAR;
   message.proto.meta.protobuf_bin = Pigweed::Protobuf::Binary::ONE;
   message.proto.meta.pigweed_bin = Pigweed::Pigweed::Binary::ONE;
@@ -1667,7 +1666,7 @@ TEST(CodegenMessage, WriteNestedForcedCallback) {
   // pigweed.device_info has use_callback=true to force the use of a callback.
   message.device_info.SetEncoder([](Pigweed::StreamEncoder& encoder) {
     DeviceInfo::Message device_info{};
-    pw::string::Copy("pixel", device_info.device_name).IgnoreError();
+    device_info.device_name = "pixel";
     device_info.device_id = 0x08080808u;
     device_info.status = DeviceInfo::DeviceStatus::OK;
 
@@ -1676,12 +1675,12 @@ TEST(CodegenMessage, WriteNestedForcedCallback) {
         [](DeviceInfo::StreamEncoder& device_info_encoder) {
           KeyValuePair::Message attribute{};
 
-          pw::string::Copy("version", attribute.key).IgnoreError();
-          pw::string::Copy("5.3.1", attribute.value).IgnoreError();
+          attribute.key = "version";
+          attribute.value = "5.3.1";
           PW_TRY(device_info_encoder.GetAttributesEncoder().Write(attribute));
 
-          pw::string::Copy("chip", attribute.key).IgnoreError();
-          pw::string::Copy("left-soc", attribute.value).IgnoreError();
+          attribute.key = "chip";
+          attribute.value = "left-soc";
           PW_TRY(device_info_encoder.GetAttributesEncoder().Write(attribute));
 
           return OkStatus();

pw_protobuf/docs.rst

@@ -1042,14 +1042,13 @@ generated ``Message`` structure into an in-memory buffer.
   #include "pw_bytes/span.h"
   #include "pw_protobuf/encoder.h"
   #include "pw_status/status_with_size.h"
-  #include "pw_string/vector.h"
 
   // Writes a proto response to the provided buffer, returning the encode
   // status and number of bytes written.
   pw::StatusWithSize WriteProtoResponse(pw::ByteSpan response) {
     MyProto::Message message{}
     message.magic_number = 0x1a1a2b2b;
-    pw::string::Copy("cookies", message.favorite_food);
+    message.favorite_food = "cookies";
     message.calories = 600;
 
     // All proto writes are directly written to the `response` buffer.

pw_string/BUILD.bazel

@@ -108,20 +108,6 @@ pw_cc_library(
     ],
 )
 
-pw_cc_library(
-    name = "vector",
-    hdrs = [
-        "public/pw_string/vector.h",
-    ],
-    includes = ["public"],
-    deps = [
-        ":pw_string",
-        ":string",
-        "//pw_containers:vector",
-        "//pw_status",
-    ],
-)
-
 pw_cc_test(
     name = "format_test",
     srcs = ["format_test.cc"],
@@ -179,13 +165,3 @@ pw_cc_test(
         "//pw_unit_test",
     ],
 )
-
-pw_cc_test(
-    name = "vector_test",
-    srcs = ["vector_test.cc"],
-    deps = [
-        ":vector",
-        "//pw_containers:vector",
-        "//pw_unit_test",
-    ],
-)

pw_string/BUILD.gn

@@ -120,18 +120,6 @@ pw_source_set("util") {
   ]
 }
 
-pw_source_set("vector") {
-  public_configs = [ ":public_include_path" ]
-  public = [ "public/pw_string/vector.h" ]
-  public_deps = [
-    ":pw_string",
-    ":string",
-    "$dir_pw_containers:vector",
-    "$dir_pw_status",
-    dir_pw_span,
-  ]
-}
-
 pw_test_group("tests") {
   tests = [
     ":string_test",
@@ -140,7 +128,6 @@ pw_test_group("tests") {
     ":to_string_test",
     ":type_to_string_test",
     ":util_test",
-    ":vector_test",
   ]
   group_deps = [
     "$dir_pw_preprocessor:tests",
@@ -182,14 +169,6 @@ pw_test("util_test") {
   sources = [ "util_test.cc" ]
 }
 
-pw_test("vector_test") {
-  deps = [
-    ":vector",
-    "$dir_pw_containers:vector",
-  ]
-  sources = [ "vector_test.cc" ]
-}
-
 pw_doc_group("docs") {
   sources = [ "docs.rst" ]
   report_deps = [

pw_string/CMakeLists.txt

@@ -102,17 +102,6 @@ pw_add_module_library(pw_string.util
     public/pw_string/internal/length.h
 )
 
-pw_add_module_library(pw_string.vector
-  HEADERS
-    public/pw_string/vector.h
-  PUBLIC_INCLUDES
-    public
-  PUBLIC_DEPS
-    pw_containers.vector
-    pw_status
-    pw_string
-)
-
 pw_add_test(pw_string.format_test
   SOURCES
     format_test.cc
@@ -175,17 +164,6 @@ pw_add_test(pw_string.util_test
     pw_string
 )
 
-pw_add_test(pw_string.vector_test
-  SOURCES
-    vector_test.cc
-  DEPS
-    pw_containers.vector
-    pw_string.vector
-  GROUPS
-    modules
-    pw_string
-)
-
 if(Zephyr_FOUND AND CONFIG_PIGWEED_STRING)
   zephyr_link_libraries(pw_string)
 endif()

pw_string/public/pw_string/util.h

@@ -28,6 +28,23 @@
 
 namespace pw {
 namespace string {
+namespace internal {
+
+PW_CONSTEXPR_CPP20 inline StatusWithSize CopyToSpan(
+    const std::string_view& source, span<char> dest) {
+  if (dest.empty()) {
+    return StatusWithSize::ResourceExhausted();
+  }
+
+  const size_t copied = source.copy(dest.data(), dest.size() - 1);
+  dest[copied] = '\0';
+
+  return StatusWithSize(
+      copied == source.size() ? OkStatus() : Status::ResourceExhausted(),
+      copied);
+}
+
+}  // namespace internal
 
 // Safe alternative to the string_view constructor to avoid the risk of an
 // unbounded implicit or explicit use of strlen.
@@ -75,24 +92,22 @@ constexpr Result<size_t> NullTerminatedLength(const char* str, size_t max_len) {
 //
 // Precondition: The destination and source shall not overlap.
 // Precondition: The source shall be a valid pointer.
+template <typename Span>
 PW_CONSTEXPR_CPP20 inline StatusWithSize Copy(const std::string_view& source,
-                                              span<char> dest) {
-  if (dest.empty()) {
-    return StatusWithSize::ResourceExhausted();
-  }
-
-  const size_t copied = source.copy(dest.data(), dest.size() - 1);
-  dest[copied] = '\0';
-
-  return StatusWithSize(
-      copied == source.size() ? OkStatus() : Status::ResourceExhausted(),
-      copied);
+                                              Span&& dest) {
+  static_assert(
+      !std::is_base_of_v<InlineString<>, std::decay_t<Span>>,
+      "Do not use pw::string::Copy() with pw::InlineString<>. Instead, use "
+      "pw::InlineString<>'s assignment operator or assign() function, or "
+      "pw::string::Append().");
+  return internal::CopyToSpan(source, std::forward<Span>(dest));
 }
 
-PW_CONSTEXPR_CPP20 inline StatusWithSize Copy(const char* source,
-                                              span<char> dest) {
+template <typename Span>
+PW_CONSTEXPR_CPP20 inline StatusWithSize Copy(const char* source, Span&& dest) {
   PW_DASSERT(source != nullptr);
-  return Copy(ClampedCString(source, dest.size()), dest);
+  return Copy(ClampedCString(source, std::size(dest)),
+              std::forward<Span>(dest));
 }
 
 PW_CONSTEXPR_CPP20 inline StatusWithSize Copy(const char* source,