Skip to content

[C++] Refactor util::Status#8354

Merged
acozzette merged 4 commits intoprotocolbuffers:masterfrom
Yannic:refactor_status
Mar 12, 2021
Merged

[C++] Refactor util::Status#8354
acozzette merged 4 commits intoprotocolbuffers:masterfrom
Yannic:refactor_status

Conversation

@Yannic
Copy link
Copy Markdown
Contributor

@Yannic Yannic commented Feb 28, 2021

This change refactors util::Status to have a similar shape as the
recently open-sourced absl::Status. This will allow Protobuf to
eventually use absl::Status and reduce the codesize.

Note that there is more work required before absl::Status can be used.

@google-cla google-cla bot added the cla: yes label Feb 28, 2021
acozzette
acozzette reviewed Mar 2, 2021
View reviewed changes
src/google/protobuf/stubs/internal/status.cc
@@ -37,61 +37,64 @@
namespace google {
namespace protobuf {
namespace util {
namespace error {
Copy link
Copy Markdown

@acozzette acozzette Mar 2, 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 moving this to a new internal subdirectory?

Copy link
Copy Markdown
Contributor Author

@Yannic Yannic Mar 3, 2021

Choose a reason for hiding this comment

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

Just stylistic preference. We can keep the implementation in this file if you'd like, with or without namespace status_internal (although I'd prefer keeping the namespace to already give a hint that the symbols will come from a different namespace eventually)

Copy link
Copy Markdown

@acozzette acozzette Mar 4, 2021

Choose a reason for hiding this comment

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

OK, I think it would be good to keep the code in the same files but moving them into a status_internal namespace sounds good.

Copy link
Copy Markdown
Contributor Author

@Yannic Yannic Mar 4, 2021

Choose a reason for hiding this comment

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

Alright, done!

Yannic
Yannic commented Mar 3, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor Author

@Yannic Yannic left a comment

Choose a reason for hiding this comment

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

Thanks for running presubmit!

I tried looking at the logs but it seems like I don't have permissions.

src/google/protobuf/stubs/internal/status.cc
@@ -37,61 +37,64 @@
namespace google {
namespace protobuf {
namespace util {
namespace error {
Copy link
Copy Markdown
Contributor Author

@Yannic Yannic Mar 3, 2021

Choose a reason for hiding this comment

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

Just stylistic preference. We can keep the implementation in this file if you'd like, with or without namespace status_internal (although I'd prefer keeping the namespace to already give a hint that the symbols will come from a different namespace eventually)

@acozzette
Copy link
Copy Markdown

acozzette commented Mar 9, 2021

The CI run is showing an error like this:

src/google/protobuf/util/internal/json_stream_parser_test.cc:153:38: error: 'class google::protobuf::util::status_internal::Status' has no member named 'error_message'
     StringPiece error_message(result.error_message());
                                      ^

Yannic added 3 commits March 9, 2021 22:09
@Yannic
[C++] Refactor util::Status
59ea5c8 
This change refactors util::Status to have a similar shape as the
recently open-sourced absl::Status. This will allow Protobuf to
eventually use absl::Status and reduce the codesize.

Note that there is more work required before absl::Status can be used.
@Yannic
Revert moving status.cc
de53e1a
@Yannic
Fix build error
baef01b
@Yannic Yannic force-pushed the refactor_status branch from 3000a23 to baef01b Compare March 9, 2021 21:19
@Yannic
Copy link
Copy Markdown
Contributor Author

Yannic commented Mar 9, 2021

The CI run is showing an error like this:

src/google/protobuf/util/internal/json_stream_parser_test.cc:153:38: error: 'class google::protobuf::util::status_internal::Status' has no member named 'error_message'
     StringPiece error_message(result.error_message());
                                      ^

Thanks! Looks like there was a second use of result.error_message() added there recently, rebased and fixed, PTAL

@acozzette
Copy link
Copy Markdown

acozzette commented Mar 9, 2021

Thanks @Yannic, it looks like there are a couple more call sites to fix in the conformance tests:

conformance_cpp.cc:137:54: error: no member named 'error_message' in 'google::protobuf::util::status_internal::Status'
                                  std::string(status.error_message()));
                                              ~~~~~~ ^
conformance_cpp.cc:192:32: error: no member named 'error_message' in 'google::protobuf::util::status_internal::Status'
            std::string(status.error_message()));

@Yannic
Copy link
Copy Markdown
Contributor Author

Yannic commented Mar 10, 2021

Thanks @Yannic, it looks like there are a couple more call sites to fix in the conformance tests:

conformance_cpp.cc:137:54: error: no member named 'error_message' in 'google::protobuf::util::status_internal::Status'
                                  std::string(status.error_message()));
                                              ~~~~~~ ^
conformance_cpp.cc:192:32: error: no member named 'error_message' in 'google::protobuf::util::status_internal::Status'
            std::string(status.error_message()));

Thanks, fixed

@acozzette acozzette merged commit 9ad9762 into protocolbuffers:master Mar 12, 2021
@Yannic Yannic deleted the refactor_status branch March 14, 2021 10:57
@veblush veblush mentioned this pull request Mar 16, 2021
Merged
@moetayuko moetayuko mentioned this pull request Dec 10, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@acozzette acozzette acozzette left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

c++ cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Yannic @acozzette @protobuf-kokoro