[C++] Refactor StatusOr and StringPiece #8406
Conversation
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
| inline bool operator==(StringPiece x, StringPiece y) { | ||
| stringpiece_ssize_type len = x.size(); | ||
| StringPiece::difference_type len = x.size(); | ||
| if (len != y.size()) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Changed to size_type (although I wasn't able to repro the error locally). PTAL, thanks!
|
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: |
|
I see there are still just a couple warnings left relating to some asserts involving size_t: It might make sense to just delete those asserts since with a |
|
Thanks, removed! PTAL |
src/google/protobuf/stubs/statusor.h
Outdated
| // 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(); |
There was a problem hiding this comment.
It looks like ConsumeValue* no longer exists anymore; maybe just delete this example.
| const StringPiece a("abcdefghijklmnopqrstuvwxyz"); | ||
| const StringPiece b("abc"); | ||
| const StringPiece c("xyz"); | ||
| StringPiece d("foobar"); |
There was a problem hiding this comment.
What's the rationale for removing d and the test assertions that use it?
There was a problem hiding this comment.
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)
|
Thanks, @Yannic! |
| const char* ptr_; | ||
| stringpiece_ssize_type length_; | ||
|
|
||
| // Prevent overflow in debug mode or fortified mode. |
There was a problem hiding this comment.
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?
4 participants
This change makes
StatusOrandStringPiecemore likeabsl::StatusOrand{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