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

Replace boost::{bind,cref,ref} with std:: version; C++11 compliant #6353

Merged
merged 1 commit into from Nov 12, 2014

Conversation

davidlt
Copy link
Contributor

@davidlt davidlt commented Nov 12, 2014

Boost 1.53.0 modified their cref/ref to match C++11 standard. Remove
dependency on boost and use standard library features. According to C++
standard you are not allowed to use cref/ref with rvalues. In this
particular case you are not allowed to pass e.streamID() as a
reference. I think, std::reference_wrapper is not allowed to hold a
reference to a temporary, otherwise it would be undefined behavior.

Details: http://wg21.cmeerw.net/lwg/issue688

Signed-off-by: David Abdurachmanov David.Abdurachmanov@cern.ch

Boost 1.53.0 modified their `cref/ref` to match C++11 standard. Remove
dependency on boost and use standard library features. According to C++
standard you are not allowed to use `cref/ref` with rvalues. In this
particular case you are not allowed to pass `e.streamID()` as a
reference. I think, `std::reference_wrapper` is not allowed to hold a
reference to a temporary, otherwise it would be undefined behavior.

Details: http://wg21.cmeerw.net/lwg/issue688

Signed-off-by: David Abdurachmanov <David.Abdurachmanov@cern.ch>
@cmsbuild
Copy link
Contributor

A new Pull Request was created by @davidlt for CMSSW_7_3_X.

Replace boost::{bind,cref,ref} with std:: version; C++11 compliant

It involves the following packages:

Mixing/Base
SimGeneral/DataMixingModule
SimGeneral/MixingModule

@cmsbuild, @civanch, @nclopezo, @mdhildreth can you please review it and eventually sign? Thanks.
@wmtan this is something you requested to watch as well.
You can sign-off by replying to this message having '+1' in the first line of your reply.
You can reject by replying to this message having '-1' in the first line of your reply.
@nclopezo you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@wmtan
Copy link
Contributor

wmtan commented Nov 12, 2014

I don't want to stop this from being merged, but I think (not 100% sure) that std::ref(_this) and std::cref(_this), when used right after the function in the std::bind of a member function, are sub-optimal. A simple "this" would be much better. All the use of ref or cref does is, at most, to save the copying of a pointer, and it may not even do that. The cost is a function call and more complex code.

@davidlt
Copy link
Contributor Author

davidlt commented Nov 12, 2014

I only made minimal changes, I kept them as-is. ref/cref will not generate a function call (they are inline by default), but there could be some overhead based on reference_wrapper. I am fine, if someone just change it to this.

@wmtan
Copy link
Contributor

wmtan commented Nov 12, 2014

I think I'll leave this alone for now. If "std::(c)ref(*this)" is changed to "this", it should be done separately from any boost->std conversion.
I agree minimal changes are best for now.

@Dr15Jones
Copy link
Contributor

Since we are talking about 'style', I think it would probably be clearer to replace all of these *::bind with C++11 lambdas.

@mdhildreth
Copy link
Contributor

Ok with me. As long as we go back and revisit this for optimisation. Someone help me remember to do that...

@mdhildreth
Copy link
Contributor

+1

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_3_X IBs unless changes (tests are also fine). This pull request will be automatically merged.

cmsbuild added a commit that referenced this pull request Nov 12, 2014
Replace boost::{bind,cref,ref} with std:: version; C++11 compliant
@cmsbuild cmsbuild merged commit 3703b25 into cms-sw:CMSSW_7_3_X Nov 12, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants