address review comments

Author: scotthart

google/cloud/spanner/uuid.cc

@@ -24,13 +24,12 @@ namespace spanner {
 GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN
 namespace {
 
-constexpr int kMaxUuidNumberOfHexDigits = 32;
-
 // Helper function to parse a single hexadecimal block of a UUID.
 // A hexadecimal block is a 16-digit hexadecimal number, which is represented
 // as 8 bytes.
 StatusOr<std::uint64_t> ParseHexBlock(absl::string_view& str,
                                       absl::string_view original_str) {
+  constexpr int kMaxUuidNumberOfHexDigits = 32;
   constexpr int kMaxUuidBlockLength = 16;
   static auto const* char_to_hex = new absl::flat_hash_map<char, std::uint8_t>(
       {{'0', 0x00}, {'1', 0x01}, {'2', 0x02}, {'3', 0x03}, {'4', 0x04},
@@ -73,14 +72,6 @@ Uuid::Uuid(absl::uint128 value) : uuid_(value) {}
 Uuid::Uuid(std::uint64_t high_bits, std::uint64_t low_bits)
     : Uuid(absl::MakeUint128(high_bits, low_bits)) {}
 
-bool operator==(Uuid const& lhs, Uuid const& rhs) {
-  return lhs.uuid_ == rhs.uuid_;
-}
-
-bool operator<(Uuid const& lhs, Uuid const& rhs) {
-  return lhs.uuid_ < rhs.uuid_;
-}
-
 Uuid::operator std::string() const {
   constexpr int kUuidStringLen = 36;
   constexpr int kChunkLength[] = {8, 4, 4, 4, 12};
@@ -95,7 +86,7 @@ Uuid::operator std::string() const {
 
   std::string output;
   output.resize(kUuidStringLen);
-  char* target = &((output)[output.size() - kUuidStringLen]);
+  char* target = const_cast<char*>(output.data());
   char* const last = &((output)[output.size()]);
   auto bits = Uint128High64(uuid_);
   int start = 16;
@@ -131,7 +122,7 @@ StatusOr<Uuid> MakeUuid(absl::string_view str) {
   }
 
   // Check for leading hyphen after stripping any surrounding braces.
-  if (str[0] == '-') {
+  if (absl::StartsWith(str, "-")) {
     return internal::InvalidArgumentError(
         absl::StrFormat("UUID cannot begin with '-': %s", original_str),
         GCP_ERROR_INFO());
@@ -144,8 +135,7 @@ StatusOr<Uuid> MakeUuid(absl::string_view str) {
 
   if (!str.empty()) {
     return internal::InvalidArgumentError(
-        absl::StrFormat("Extra characters (%d) found after parsing UUID: %s",
-                        str.size(), original_str),
+        absl::StrFormat("Extra characters found after parsing UUID: %s", str),
         GCP_ERROR_INFO());
   }
 

google/cloud/spanner/uuid.h

@@ -38,10 +38,10 @@ class Uuid {
   /// Default construction yields a zero value UUID.
   Uuid() = default;
 
-  /// Construct a UUID from a packed integer.
+  /// Construct a UUID from one 128 bit unsigned integer.
   explicit Uuid(absl::uint128 value);
 
-  /// Construct a UUID from two 64 bit pieces.
+  /// Construct a UUID from two unsigned 64 bit pieces.
   Uuid(std::uint64_t high_bits, std::uint64_t low_bits);
 
   /// @name Regular value type, supporting copy, assign, move.
@@ -55,11 +55,15 @@ class Uuid {
   /// @name Relational operators
   ///
   ///@{
-  friend bool operator==(Uuid const& lhs, Uuid const& rhs);
+  friend bool operator==(Uuid const& lhs, Uuid const& rhs) {
+    return lhs.uuid_ == rhs.uuid_;
+  }
   friend bool operator!=(Uuid const& lhs, Uuid const& rhs) {
     return !(lhs == rhs);
   }
-  friend bool operator<(Uuid const& lhs, Uuid const& rhs);
+  friend bool operator<(Uuid const& lhs, Uuid const& rhs) {
+    return lhs.uuid_ < rhs.uuid_;
+  }
   friend bool operator<=(Uuid const& lhs, Uuid const& rhs) {
     return !(rhs < lhs);
   }
@@ -91,10 +95,9 @@ class Uuid {
  * Parses a textual representation a `Uuid` from a string of hexadecimal digits.
  * Returns an error if unable to parse the given input.
  *
- * Acceptable input strings must consist of [0-f] digits with formatting such:
- *  - Upper, lower, or mixed case.
- *  - Optional curly bracers around the entire UUID string.
- *  - Hyphens between any pair of hexadecimal digits are allowed.
+ * Acceptable input strings must consist of 32 hexadecimal digits: [0-9a-fA-F].
+ * Optional curly braces are allowed around the entire sequence of digits as are
+ * hyphens between any pair of hexadecimal digits.
  *
  * Example acceptable inputs:
  *  - {0b6ed04c-a16d-fc46-5281-7f9978c13738}

google/cloud/spanner/uuid_test.cc

@@ -118,12 +118,14 @@ INSTANTIATE_TEST_SUITE_P(
         MakeUuidTestParam{
             "MissingClosingCurlyBrace", "{0b6ed04ca16dfc4652817f9978c13738",
             0x0, 0x0,
-            Status{StatusCode::kInvalidArgument, "UUID missing closing '}':"}},
+            Status{
+                StatusCode::kInvalidArgument,
+                "UUID missing closing '}': {0b6ed04ca16dfc4652817f9978c13738"}},
         MakeUuidTestParam{
             "MissingOpeningCurlyBrace", "0b6ed04ca16dfc4652817f9978c13738}",
             0x0, 0x0,
             Status{StatusCode::kInvalidArgument,
-                   "Extra characters (1) found after parsing UUID:"}},
+                   "Extra characters found after parsing UUID: }"}},
         MakeUuidTestParam{"StartsWithInvalidHyphen",
                           "-0b6ed04ca16dfc4652817f9978c13738", 0x0, 0x0,
                           Status{StatusCode::kInvalidArgument,
@@ -135,7 +137,8 @@ INSTANTIATE_TEST_SUITE_P(
         MakeUuidTestParam{"ContainsConsecutiveHyphens",
                           "0b--6ed04ca16dfc4652817f9978c13738", 0x0, 0x0,
                           Status{StatusCode::kInvalidArgument,
-                                 "UUID cannot contain consecutive hyphens:"}},
+                                 "UUID cannot contain consecutive hyphens: "
+                                 "0b--6ed04ca16dfc4652817f9978c13738"}},
         MakeUuidTestParam{"InsufficientDigits",
                           "00-00-00-00-00-00-00-00-00-00-00-00-00-00-00", 0x0,
                           0x0,