refactor(GCS+gRPC): move MakeCord() helpers (#12724)

Move the `MakeCord(std::string)`, and `MakeCord(std::vector<T>)` helpers
to their own header. I want to use this outside the `AsyncClient`
implementation.

Author: coryan

google/cloud/internal/random.h

@@ -16,6 +16,7 @@
 #define GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_INTERNAL_RANDOM_H
 
 #include "google/cloud/version.h"
+#include <algorithm>
 #include <random>
 #include <string>
 #include <vector>
@@ -58,6 +59,35 @@ inline DefaultPRNG MakeDefaultPRNG() {
  */
 std::string Sample(DefaultPRNG& gen, int n, std::string const& population);
 
+template <typename Collection,
+          typename std::enable_if<std::is_same<Collection, std::string>::value,
+                                  int>::type = 0>
+std::string RandomDataToCollection(std::string v) {
+  // This is not motivated by a desire to optimize this function (though that is
+  // nice). The issue is that I (coryan@) could not figure out how to write a
+  // generic version of this function that works with `std::vector<std::byte>`
+  // and `std::string`.
+  return v;
+}
+
+template <typename Collection,
+          typename std::enable_if<!std::is_same<Collection, std::string>::value,
+                                  int>::type = 0>
+Collection RandomDataToCollection(std::string v) {
+  Collection result(v.size());
+  std::transform(v.begin(), v.end(), result.begin(), [](auto c) {
+    return static_cast<typename Collection::value_type>(c);
+  });
+  return result;
+}
+
+template <typename Collection>
+Collection RandomData(DefaultPRNG& generator, std::size_t size) {
+  auto data = Sample(generator, static_cast<int>(size),
+                     "abcdefghijklmnopqrstuvwxyz0123456789");
+  return RandomDataToCollection<Collection>(std::move(data));
+}
+
 }  // namespace internal
 GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END
 }  // namespace cloud

google/cloud/storage/google_cloud_cpp_storage_grpc.bzl

@@ -45,6 +45,7 @@ google_cloud_cpp_storage_grpc_hdrs = [
     "internal/grpc/ctype_cord_workaround.h",
     "internal/grpc/hmac_key_metadata_parser.h",
     "internal/grpc/hmac_key_request_parser.h",
+    "internal/grpc/make_cord.h",
     "internal/grpc/notification_metadata_parser.h",
     "internal/grpc/notification_request_parser.h",
     "internal/grpc/object_access_control_parser.h",
@@ -87,6 +88,7 @@ google_cloud_cpp_storage_grpc_srcs = [
     "internal/grpc/configure_client_context.cc",
     "internal/grpc/hmac_key_metadata_parser.cc",
     "internal/grpc/hmac_key_request_parser.cc",
+    "internal/grpc/make_cord.cc",
     "internal/grpc/notification_metadata_parser.cc",
     "internal/grpc/notification_request_parser.cc",
     "internal/grpc/object_access_control_parser.cc",

google/cloud/storage/google_cloud_cpp_storage_grpc.cmake

@@ -85,6 +85,8 @@ else ()
         internal/grpc/hmac_key_metadata_parser.h
         internal/grpc/hmac_key_request_parser.cc
         internal/grpc/hmac_key_request_parser.h
+        internal/grpc/make_cord.cc
+        internal/grpc/make_cord.h
         internal/grpc/notification_metadata_parser.cc
         internal/grpc/notification_metadata_parser.h
         internal/grpc/notification_request_parser.cc
@@ -285,6 +287,7 @@ if (BUILD_TESTING AND GOOGLE_CLOUD_CPP_STORAGE_ENABLE_GRPC)
         internal/grpc/configure_client_context_test.cc
         internal/grpc/hmac_key_metadata_parser_test.cc
         internal/grpc/hmac_key_request_parser_test.cc
+        internal/grpc/make_cord_test.cc
         internal/grpc/notification_metadata_parser_test.cc
         internal/grpc/notification_request_parser_test.cc
         internal/grpc/object_access_control_parser_test.cc

google/cloud/storage/internal/async/write_payload_impl.cc

@@ -20,16 +20,6 @@ namespace cloud {
 namespace storage_internal {
 GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN
 
-absl::Cord MakeCord(std::string p) {
-  // The absl::Cord constructor from `std::string` splits the string into many
-  // small buffers (and allocates a container for each). We want to avoid copies
-  // and allocations.
-  auto holder = std::make_shared<std::string>(std::move(p));
-  auto contents = absl::string_view(holder->data(), holder->size());
-  return absl::MakeCordFromExternal(contents,
-                                    [b = std::move(holder)]() mutable {});
-}
-
 storage_experimental::WritePayload MakeWritePayload(std::string p) {
   return WritePayloadImpl::Make(MakeCord(std::move(p)));
 }

google/cloud/storage/internal/async/write_payload_impl.h

@@ -16,11 +16,9 @@
 #define GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_STORAGE_INTERNAL_ASYNC_WRITE_PAYLOAD_IMPL_H
 
 #include "google/cloud/storage/async_object_requests.h"
+#include "google/cloud/storage/internal/grpc/make_cord.h"
 #include "google/cloud/version.h"
-#include "absl/meta/type_traits.h"
-#include <algorithm>
-#include <cstddef>
-#include <cstdint>
+#include "absl/strings/cord.h"
 #include <numeric>
 
 namespace google {
@@ -39,28 +37,6 @@ struct WritePayloadImpl {
   }
 };
 
-template <typename T>
-using IsPayloadType = absl::disjunction<
-#if GOOGLE_CLOUD_CPP_CPP_VERSION >= 201703L
-    std::is_same<T, std::byte>,
-#endif
-    std::is_same<T, char>, std::is_same<T, signed char>,
-    std::is_same<T, unsigned char>, std::is_same<T, std::uint8_t>>;
-
-/// Create an `absl::Cord`, without copying the data in @p p.
-absl::Cord MakeCord(std::string p);
-
-/// Create an `absl::Cord`, without copying the data in @p p.
-template <typename T>
-absl::Cord MakeCord(std::vector<T> p) {
-  static_assert(IsPayloadType<T>::value, "unexpected value type");
-  auto holder = std::make_shared<std::vector<T>>(std::move(p));
-  auto contents = absl::string_view(
-      reinterpret_cast<char const*>(holder->data()), holder->size());
-  return absl::MakeCordFromExternal(contents,
-                                    [b = std::move(holder)]() mutable {});
-}
-
 storage_experimental::WritePayload MakeWritePayload(std::string p);
 template <typename T,
           typename std::enable_if<IsPayloadType<T>::value, int>::type = 0>

google/cloud/storage/internal/async/write_payload_impl_test.cc

@@ -23,39 +23,9 @@ namespace storage_internal {
 GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN
 namespace {
 
-using ::testing::ElementsAre;
 using ::testing::IsEmpty;
 using ::testing::Not;
 
-template <typename Collection,
-          typename std::enable_if<std::is_same<Collection, std::string>::value,
-                                  int>::type = 0>
-Collection ToCollection(std::string v) {
-  // This is not motivated by a desire to optimize this test helper function
-  // (though that is nice). The issue is that I (coryan@) could not figure out
-  // how to write a generic version of this function that works with
-  // `std::vector<std::byte>` and `std::string`.
-  return v;
-}
-
-template <typename Collection,
-          typename std::enable_if<!std::is_same<Collection, std::string>::value,
-                                  int>::type = 0>
-Collection ToCollection(std::string v) {
-  Collection result(v.size());
-  std::transform(v.begin(), v.end(), result.begin(), [](auto c) {
-    return static_cast<typename Collection::value_type>(c);
-  });
-  return result;
-}
-
-template <typename Collection>
-Collection RandomData(internal::DefaultPRNG& generator, std::size_t size) {
-  auto data = internal::Sample(generator, static_cast<int>(size),
-                               "abcdefghijklmnopqrstuvwxyz0123456789");
-  return ToCollection<Collection>(std::move(data));
-}
-
 TEST(WritePayloadImpl, Default) {
   auto const actual = storage_experimental::WritePayload();
   EXPECT_TRUE(actual.empty());
@@ -81,13 +51,6 @@ TEST(WritePayloadImpl, GetImpl) {
   EXPECT_EQ(WritePayloadImpl::GetImpl(actual), absl::Cord(expected));
 }
 
-TEST(WritePayloadImpl, IsPayloadType) {
-  EXPECT_TRUE(IsPayloadType<char>::value);
-  EXPECT_TRUE(IsPayloadType<std::uint8_t>::value);
-  EXPECT_FALSE(IsPayloadType<std::string>::value);
-  EXPECT_FALSE(IsPayloadType<std::uint32_t>::value);
-}
-
 template <typename T>
 class Typed : public ::testing::Test {
  public:
@@ -103,26 +66,14 @@ using TestVariations = ::testing::Types<
 
 TYPED_TEST_SUITE(Typed, TestVariations);
 
-TYPED_TEST(Typed, MakeCord) {
-  using Collection = typename TestFixture::TestType;
-  static auto generator = internal::MakeDefaultPRNG();
-  auto constexpr kTestDataSize = 8 * 1024 * 1024L;
-  auto const buffer = RandomData<Collection>(generator, kTestDataSize);
-  auto const view = absl::string_view(
-      reinterpret_cast<char const*>(buffer.data()), buffer.size());
-
-  auto const actual = MakeCord(buffer);
-  auto chunks = actual.Chunks();
-  EXPECT_THAT(chunks, ElementsAre(view));
-}
-
 TYPED_TEST(Typed, MakeWritePayload) {
   using Collection = typename TestFixture::TestType;
 
   auto const expected =
       std::string{"The quick brown fox jumps over the lazy dog"};
 
-  auto const actual = MakeWritePayload(ToCollection<Collection>(expected));
+  auto const actual =
+      MakeWritePayload(internal::RandomDataToCollection<Collection>(expected));
   EXPECT_FALSE(actual.empty());
   EXPECT_EQ(actual.size(), expected.size());
   EXPECT_THAT(actual.payload(), Not(IsEmpty()));
@@ -135,11 +86,12 @@ TYPED_TEST(Typed, MakeWritePayloadFromVector) {
 
   static auto generator = internal::MakeDefaultPRNG();
   auto constexpr kTestDataSize = 1024;
-  auto const part = RandomData<std::string>(generator, kTestDataSize);
+  auto const part = internal::RandomData<std::string>(generator, kTestDataSize);
   auto const expected = part + part + part;
   auto const actual = MakeWritePayload(std::vector<Collection>{
-      {ToCollection<Collection>(part), ToCollection<Collection>(part),
-       ToCollection<Collection>(part)}});
+      {internal::RandomDataToCollection<Collection>(part),
+       internal::RandomDataToCollection<Collection>(part),
+       internal::RandomDataToCollection<Collection>(part)}});
   EXPECT_FALSE(actual.empty());
   EXPECT_EQ(actual.size(), expected.size());
   EXPECT_THAT(actual.payload(), Not(IsEmpty()));

google/cloud/storage/internal/grpc/make_cord.cc

@@ -0,0 +1,36 @@
+// Copyright 2023 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//      https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include "google/cloud/storage/internal/grpc/make_cord.h"
+#include <utility>
+
+namespace google {
+namespace cloud {
+namespace storage_internal {
+GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN
+
+absl::Cord MakeCord(std::string p) {
+  // The absl::Cord constructor from `std::string` splits the string into many
+  // small buffers (and allocates a container for each). We want to avoid copies
+  // and allocations.
+  auto holder = std::make_shared<std::string>(std::move(p));
+  auto contents = absl::string_view(holder->data(), holder->size());
+  return absl::MakeCordFromExternal(contents,
+                                    [b = std::move(holder)]() mutable {});
+}
+
+GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END
+}  // namespace storage_internal
+}  // namespace cloud
+}  // namespace google

google/cloud/storage/internal/grpc/make_cord.h

@@ -0,0 +1,62 @@
+// Copyright 2023 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//      https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#ifndef GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_STORAGE_INTERNAL_GRPC_MAKE_CORD_H
+#define GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_STORAGE_INTERNAL_GRPC_MAKE_CORD_H
+
+#include "google/cloud/version.h"
+#include "absl/meta/type_traits.h"
+#include "absl/strings/cord.h"
+#include "absl/strings/string_view.h"
+#include <cstddef>
+#include <cstdint>
+#include <memory>
+#include <string>
+#include <type_traits>
+#include <utility>
+#include <vector>
+
+namespace google {
+namespace cloud {
+namespace storage_internal {
+GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN
+
+template <typename T>
+using IsPayloadType = absl::disjunction<
+#if GOOGLE_CLOUD_CPP_CPP_VERSION >= 201703L
+    std::is_same<T, std::byte>,
+#endif
+    std::is_same<T, char>, std::is_same<T, signed char>,
+    std::is_same<T, unsigned char>, std::is_same<T, std::uint8_t>>;
+
+/// Create an `absl::Cord`, without copying the data in @p p.
+absl::Cord MakeCord(std::string p);
+
+/// Create an `absl::Cord`, without copying the data in @p p.
+template <typename T>
+absl::Cord MakeCord(std::vector<T> p) {
+  static_assert(IsPayloadType<T>::value, "unexpected value type");
+  auto holder = std::make_shared<std::vector<T>>(std::move(p));
+  auto contents = absl::string_view(
+      reinterpret_cast<char const*>(holder->data()), holder->size());
+  return absl::MakeCordFromExternal(contents,
+                                    [b = std::move(holder)]() mutable {});
+}
+
+GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END
+}  // namespace storage_internal
+}  // namespace cloud
+}  // namespace google
+
+#endif  // GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_STORAGE_INTERNAL_GRPC_MAKE_CORD_H