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

Use static_cast rather than reinterpret_cast whenever possible #14597

Merged
merged 1 commit into from
Mar 6, 2018

Conversation

vjpai
Copy link
Member

@vjpai vjpai commented Mar 5, 2018

In particular, you should use static_cast for going up or down a class hierarchy or for converting between T* and void* .

Many of the remaining uses of reinterpret_cast are for things that are "morally" part of a class hierarchy (e.g., grpc_endpoint vs mock_endpoint) but currently aren't because they are expressed in C-style. This will allow further reduction of reinterpret_cast in the future.

@grpc-testing
Copy link

[trickle] No significant performance differences

@@ -132,7 +132,7 @@ static int te_get_fd(grpc_endpoint* ep) {
}

static void te_finish_write(void* arg, grpc_error* error) {
trickle_endpoint* te = static_cast<trickle_endpoint*>(arg);
trickle_endpoint* te = reinterpret_cast<trickle_endpoint*>(arg);
Copy link
Contributor

Choose a reason for hiding this comment

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

If static_cast was compiling here, why switch to reinterpret_cast?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, I slipped. Thanks for noticing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Specifically, my method here was to s/reinterpret_cast/static_cast/g, get all the build errors and change those back with an Emacs macro. Looks like I slipped here. I'll check the others.

@grpc-testing
Copy link

[microbenchmarks] No significant performance differences

@grpc-testing
Copy link

[trickle] No significant performance differences

1 similar comment
@grpc-testing
Copy link

[trickle] No significant performance differences

@grpc-testing
Copy link

[microbenchmarks] No significant performance differences

1 similar comment
@grpc-testing
Copy link

[microbenchmarks] No significant performance differences

@yashykt
Copy link
Member

yashykt commented Mar 5, 2018

Thanks for doing this Vijay! From the errors, it looks like some of the conversions weren't part of a hierarchy or void * to and fro conversions.

LGTM once it builds :)

Copy link
Member

@yashykt yashykt left a comment

Choose a reason for hiding this comment

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

LGTM once it builds

@vjpai
Copy link
Member Author

vjpai commented Mar 5, 2018

I wasn't building Cronet until now. Hopefully this goes through now.

@grpc-testing
Copy link

****************************************************************

libgrpc.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]


****************************************************************

libgrpc++.so

     VM SIZE              FILE SIZE
 ++++++++++++++ GROWIN ++++++++++++++
  [ = ]       0 [None]      +8  +0.0%

  [ = ]       0 TOTAL       +8  +0.0%



@grpc-testing
Copy link

[trickle] No significant performance differences

@grpc-testing
Copy link

****************************************************************

libgrpc.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]


****************************************************************

libgrpc++.so

     VM SIZE              FILE SIZE
 ++++++++++++++ GROWIN ++++++++++++++
  [ = ]       0 [None]      +8  +0.0%

  [ = ]       0 TOTAL       +8  +0.0%



@grpc-testing
Copy link

[trickle] No significant performance differences

@grpc-testing
Copy link

[microbenchmarks] No significant performance differences

1 similar comment
@grpc-testing
Copy link

[microbenchmarks] No significant performance differences

@vjpai
Copy link
Member Author

vjpai commented Mar 6, 2018

Rebased, diff unchanged

@grpc-testing
Copy link

****************************************************************

libgrpc.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]


****************************************************************

libgrpc++.so

     VM SIZE              FILE SIZE
 ++++++++++++++ GROWIN ++++++++++++++
  [ = ]       0 [None]      +8  +0.0%

  [ = ]       0 TOTAL       +8  +0.0%



@grpc-testing
Copy link

[trickle] No significant performance differences

@grpc-testing
Copy link

[microbenchmarks] No significant performance differences

@vjpai vjpai merged commit 50cecca into grpc:master Mar 6, 2018
@vjpai vjpai deleted the deinterpret branch March 6, 2018 06:06
@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants