Skip to content

Enforce CRTP: weaken reinterpret_cast to static_cast#14345

Merged
vjpai merged 2 commits intogrpc:masterfrom
vjpai:static
Feb 7, 2018
Merged

Enforce CRTP: weaken reinterpret_cast to static_cast#14345
vjpai merged 2 commits intogrpc:masterfrom
vjpai:static

Conversation

@vjpai
Copy link
Copy Markdown
Contributor

@vjpai vjpai commented Feb 7, 2018

Since RefCounted is designed for use with CRTP, let these casts be static_cast (which can only do void, up, down, and nullptr casts) rather than reinterpret_cast (which can do anything). This helps to make sure that an appropriate CRTP parameter is used (the static_casts will break if misdeclared).

@vjpai vjpai requested a review from markdroth February 7, 2018 02:44
@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Feb 7, 2018 

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

libgrpc.so

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

  [ = ]       0        0  [ = ]


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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Feb 7, 2018 

[trickle] No significant performance differences

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Feb 7, 2018 

Corrupt JSON data (indicates timeout or crash): 
    bm_fullstack_streaming_pump.BM_PumpStreamServerToClient_SockPair__1.counters.new: 1


[microbenchmarks] No significant performance differences

@markdroth
Copy link
Copy Markdown
Member

markdroth commented Feb 7, 2018

Please also change this in the following places:

https://github.com/grpc/grpc/blob/master/src/core/lib/gprpp/orphanable.h#L95
https://github.com/grpc/grpc/blob/master/src/core/lib/gprpp/orphanable.h#L152

@vjpai
Copy link
Copy Markdown
Contributor Author

vjpai commented Feb 7, 2018

Mark, thanks for the feedback. Done.

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Feb 7, 2018 

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

libgrpc.so

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

  [ = ]       0        0  [ = ]


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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Feb 7, 2018 

[trickle] No significant performance differences

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Feb 7, 2018 

Corrupt JSON data (indicates timeout or crash): 
    bm_fullstack_streaming_pump.BM_PumpStreamServerToClient_SockPair__512.opt.old: 1
    bm_fullstack_streaming_pump.BM_PumpStreamServerToClient_SockPair__8.counters.old: 1


[microbenchmarks] No significant performance differences

@vjpai vjpai merged commit 0e2d9fa into grpc:master Feb 7, 2018
@vjpai vjpai deleted the static branch February 7, 2018 20:59
@lock lock bot locked as resolved and limited conversation to collaborators Jan 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@markdroth markdroth markdroth approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@vjpai @grpc-testing @markdroth