[C++] Refactor StatusOr and StringPiece #8406

Yannic · 2021-03-14T16:08:06Z

This change makes StatusOr and StringPiece more like
absl::StatusOr and {absl,std}::string_view.

Note that there is more work required before the Abseil types can be
used as drop-in replacement.

Progress on #3688

This change makes `StatusOr` and `StringPiece` more like `absl::StatusOr` and `{absl,std}::string_view`. Note that there is more work required before the Abseil types can be used as drop-in replacement. Progress on protocolbuffers#3688

acozzette · 2021-03-15T20:07:30Z

src/google/protobuf/stubs/stringpiece.h

@@ -391,7 +321,7 @@ class PROTOBUF_EXPORT StringPiece {
 // one of the arguments is a literal, the compiler can elide a lot of the
 // following comparisons.
 inline bool operator==(StringPiece x, StringPiece y) {
-  stringpiece_ssize_type len = x.size();
+  StringPiece::difference_type len = x.size();
  if (len != y.size()) {


Some of the test runs are showing an error on this line:

../../../src/google/protobuf/stubs/stringpiece.h: In function ‘bool google::protobuf::stringpiece_internal::operator==(google::protobuf::stringpiece_internal::StringPiece, google::protobuf::stringpiece_internal::StringPiece)’: ../../../src/google/protobuf/stubs/stringpiece.h:325:11: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare] if (len != y.size()) { ^

I think perhaps len should be a size_type.

Changed to size_type (although I wasn't able to repro the error locally). PTAL, thanks!

acozzette · 2021-03-17T15:53:14Z

It looks like there are just a few more errors coming from stringpiece.h in no_warning_test, which tries to make sure everything builds with warnings treated as errors:

./google/protobuf/stubs/stringpiece.h: In constructor ‘google::protobuf::stringpiece_internal::StringPiece::StringPiece(const char*, google::protobuf::stringpiece_internal::StringPiece::size_type)’:
./google/protobuf/stubs/stringpiece.h:203:19: error: comparison of unsigned expression >= 0 is always true [-Werror=type-limits]
     assert(len >= 0);
                   ^
./google/protobuf/stubs/stringpiece.h: In member function ‘char google::protobuf::stringpiece_internal::StringPiece::operator[](google::protobuf::stringpiece_internal::StringPiece::size_type) const’:
./google/protobuf/stubs/stringpiece.h:216:17: error: comparison of unsigned expression >= 0 is always true [-Werror=type-limits]
     assert(0 <= i);
                 ^
./google/protobuf/stubs/stringpiece.h: In member function ‘void google::protobuf::stringpiece_internal::StringPiece::remove_prefix(google::protobuf::stringpiece_internal::StringPiece::difference_type)’:
./google/protobuf/stubs/stringpiece.h:222:23: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]
     assert(length_ >= n);
                       ^
./google/protobuf/stubs/stringpiece.h: In member function ‘void google::protobuf::stringpiece_internal::StringPiece::remove_suffix(google::protobuf::stringpiece_internal::StringPiece::difference_type)’:
./google/protobuf/stubs/stringpiece.h:228:23: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]
     assert(length_ >= n);
                       ^

acozzette · 2021-03-18T17:13:37Z

I see there are still just a couple warnings left relating to some asserts involving size_t:

./google/protobuf/stubs/stringpiece.h: In constructor ‘google::protobuf::stringpiece_internal::StringPiece::StringPiece(const char*, google::protobuf::stringpiece_internal::StringPiece::size_type)’:
./google/protobuf/stubs/stringpiece.h:203:19: error: comparison of unsigned expression >= 0 is always true [-Werror=type-limits]
     assert(len >= 0);
                   ^
./google/protobuf/stubs/stringpiece.h: In member function ‘char google::protobuf::stringpiece_internal::StringPiece::operator[](google::protobuf::stringpiece_internal::StringPiece::size_type) const’:
./google/protobuf/stubs/stringpiece.h:216:17: error: comparison of unsigned expression >= 0 is always true [-Werror=type-limits]
     assert(0 <= i);
                 ^

It might make sense to just delete those asserts since with a size_t they are always true.

Yannic · 2021-03-18T21:26:27Z

Thanks, removed! PTAL

acozzette · 2021-03-19T17:37:46Z

src/google/protobuf/stubs/statusor.h

@@ -62,7 +62,7 @@
 //
 //  StatusOr<std::unique_ptr<Foo>> result = FooFactory::MakeNewFoo(arg);
 //  if (result.ok()) {
-//    std::unique_ptr<Foo> foo = result.ConsumeValueOrDie();
+//    std::unique_ptr<Foo> foo = result.Consumevalue();


It looks like ConsumeValue* no longer exists anymore; maybe just delete this example.

acozzette · 2021-03-19T18:12:05Z

src/google/protobuf/stubs/stringpiece_unittest.cc

@@ -305,16 +305,9 @@ TEST(StringPiece, STL2) {
  const StringPiece a("abcdefghijklmnopqrstuvwxyz");
  const StringPiece b("abc");
  const StringPiece c("xyz");
-  StringPiece d("foobar");


What's the rationale for removing d and the test assertions that use it?

d was a used to test clear() which no longer exists because {absl,std}::string_view don't have such a method. Without clear(), d becomes equivalent to e and since there are assertions against e everywhere, I removed all uses of d (compare L327+328 or L329+330)

acozzette · 2021-03-23T18:54:19Z

Thanks, @Yannic!

haberman · 2021-04-21T00:20:14Z

src/google/protobuf/stubs/stringpiece.h

 private:
  const char* ptr_;
-  stringpiece_ssize_type length_;
-
-  // Prevent overflow in debug mode or fortified mode.


This PR removed the length checking code, but if we are matching ABSL and STL they both have a max_size(). In the case of ABSL it is std::numeric_limits<difference_type>::max(): https://github.com/abseil/abseil-cpp/blob/1ae9b71c474628d60eb251a3f62967fe64151bb2/absl/strings/string_view.h#L524-L529

Can we add this checking code back with a max size to match ABSL?

[C++] Refactor StatusOr and StringPiece

2fd38af

This change makes `StatusOr` and `StringPiece` more like `absl::StatusOr` and `{absl,std}::string_view`. Note that there is more work required before the Abseil types can be used as drop-in replacement. Progress on protocolbuffers#3688

google-cla bot added the cla: yes label Mar 14, 2021

acozzette added c++ release notes: yes labels Mar 15, 2021

protobuf-kokoro removed kokoro:run labels Mar 15, 2021

acozzette reviewed Mar 15, 2021

View reviewed changes

Merge branch 'master' into refactor-statusor-stringpiece

61c8498

acozzette added the kokoro:run label Mar 16, 2021

protobuf-kokoro removed the kokoro:run label Mar 16, 2021

Fix more errors

dcb2f4b

acozzette added the kokoro:run label Mar 17, 2021

protobuf-kokoro removed the kokoro:run label Mar 17, 2021

Fix test

ff0fe7f

acozzette added the kokoro:run label Mar 18, 2021

protobuf-kokoro removed the kokoro:run label Mar 18, 2021

Remove some asserts

11dc71a

acozzette added the kokoro:run label Mar 18, 2021

protobuf-kokoro removed the kokoro:run label Mar 18, 2021

acozzette requested changes Mar 19, 2021

View reviewed changes

Delete outdate example

f00f584

acozzette added the kokoro:run label Mar 22, 2021

protobuf-kokoro removed the kokoro:run label Mar 22, 2021

acozzette merged commit 4b770ca into protocolbuffers:master Mar 23, 2021

haberman reviewed Apr 21, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[C++] Refactor StatusOr and StringPiece #8406

[C++] Refactor StatusOr and StringPiece #8406

Yannic commented Mar 14, 2021

acozzette Mar 15, 2021

Yannic Mar 16, 2021

acozzette commented Mar 17, 2021

acozzette commented Mar 18, 2021

Yannic commented Mar 18, 2021

acozzette Mar 19, 2021

Yannic Mar 20, 2021

acozzette Mar 19, 2021

Yannic Mar 20, 2021

acozzette commented Mar 23, 2021

haberman Apr 21, 2021

[C++] Refactor StatusOr and StringPiece #8406

[C++] Refactor StatusOr and StringPiece #8406

Conversation

Yannic commented Mar 14, 2021

acozzette Mar 15, 2021

Choose a reason for hiding this comment

Yannic Mar 16, 2021

Choose a reason for hiding this comment

acozzette commented Mar 17, 2021

acozzette commented Mar 18, 2021

Yannic commented Mar 18, 2021

acozzette Mar 19, 2021

Choose a reason for hiding this comment

Yannic Mar 20, 2021

Choose a reason for hiding this comment

acozzette Mar 19, 2021

Choose a reason for hiding this comment

Yannic Mar 20, 2021

Choose a reason for hiding this comment

acozzette commented Mar 23, 2021

haberman Apr 21, 2021

Choose a reason for hiding this comment