Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[C++] Refactor StatusOr and StringPiece #8406

Merged

Conversation

Yannic
Copy link
Contributor

@Yannic Yannic commented Mar 14, 2021

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
@google-cla google-cla bot added the cla: yes label Mar 14, 2021
@acozzette acozzette added c++ release notes: yes kokoro:run labels Mar 15, 2021
@@ -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()) {
Copy link
Member

@acozzette acozzette Mar 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@Yannic Yannic Mar 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@acozzette
Copy link
Member

@acozzette acozzette commented Mar 17, 2021

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
Copy link
Member

@acozzette acozzette commented Mar 18, 2021

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
Copy link
Contributor Author

@Yannic Yannic commented Mar 18, 2021

Thanks, removed! PTAL

@@ -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();
Copy link
Member

@acozzette acozzette Mar 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@Yannic Yannic Mar 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

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

@acozzette acozzette Mar 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@Yannic Yannic Mar 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 acozzette merged commit 4b770ca into protocolbuffers:master Mar 23, 2021
55 of 56 checks passed
@acozzette
Copy link
Member

@acozzette acozzette commented Mar 23, 2021

Thanks, @Yannic!

private:
const char* ptr_;
stringpiece_ssize_type length_;

// Prevent overflow in debug mode or fortified mode.
Copy link
Member

@haberman haberman Apr 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ cla: yes release notes: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants