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

[objc] GRPCErrorCode enum base type to int32_t #27908

Merged
merged 1 commit into from
Nov 2, 2021

Conversation

dennycd
Copy link
Contributor

@dennycd dennycd commented Nov 2, 2021

Updating GRPCErrorCode base enum type from NSUInteger to int32_t to have better integer size consistency across various architectures (32/64).

@dennycd dennycd added lang/ObjC platform/iOS release notes: yes Indicates if PR needs to be in release notes labels Nov 2, 2021
@@ -23,7 +23,7 @@
* Note that a few of these are never produced by the gRPC libraries, but are of
* general utility for server applications to produce.
*/
typedef NS_ENUM(NSUInteger, GRPCErrorCode) {
typedef NS_ENUM(int32_t, GRPCErrorCode) {
Copy link
Contributor

@sampajano sampajano Nov 2, 2021

Choose a reason for hiding this comment

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

Thanks Daniel! LGTM in general :)

2 thoughts:

  1. Is this a non-breaking change for all clients?

  2. There are a few other enums in this file, as well as throughout the codebase, which i wonder would benefit from the same improvement?

Thanks :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, this change is non-breaking. This PR changes just the GRPCErrorCode enum. We will follow up with incremental changes to the rest of the enum when we made corresponding g3 patches.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok thanks :)

(In general, for the sake of API consistency i'd prefer our APIs to use similar types if possible.. But it's fine with me since you're going to do a bunch of follow-ups soon :))

@dennycd dennycd enabled auto-merge (squash) November 2, 2021 22:56
@dennycd dennycd merged commit 6b91698 into grpc:master Nov 2, 2021
dennycd added a commit that referenced this pull request Nov 3, 2021
dennycd added a commit that referenced this pull request Nov 3, 2021
dennycd added a commit that referenced this pull request Nov 3, 2021
@copybara-service copybara-service bot added the imported Specifies if the PR has been imported to the internal repository label Nov 3, 2021
@dennycd dennycd added release notes: no Indicates if PR should not be in release notes and removed release notes: yes Indicates if PR needs to be in release notes labels Nov 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
imported Specifies if the PR has been imported to the internal repository lang/ObjC platform/iOS release notes: no Indicates if PR should not be in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants