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 deprecated binders and adapters, and random_shuffle by more m… #106

Merged
merged 1 commit into from May 22, 2017

Conversation

Projects
None yet
6 participants
@DanielaE
Contributor

DanielaE commented Nov 6, 2016

replace deprecated binders and adapters, and random_shuffle by more modern equivalents

Required with latest msvc versions and compiler option /std:c++latest

@raffienficiaud

This comment has been minimized.

Show comment
Hide comment
@raffienficiaud

raffienficiaud Nov 6, 2016

Member

Thanks for the patch, however your changes need C++11, which is not allowed unfortunately with our library. We have several locations where we have mixed C++98/C++11(+) code. Do you feel like you can rework your patch such that it is compatible with pre and post C++11?

Member

raffienficiaud commented Nov 6, 2016

Thanks for the patch, however your changes need C++11, which is not allowed unfortunately with our library. We have several locations where we have mixed C++98/C++11(+) code. Do you feel like you can rework your patch such that it is compatible with pre and post C++11?

@raffienficiaud raffienficiaud self-assigned this Nov 6, 2016

@DanielaE

This comment has been minimized.

Show comment
Hide comment
@DanielaE

DanielaE Nov 7, 2016

Contributor

Raffi, I think that is possible, but I need your advise on two points:

  • do you mind taking a dependency on Boost.Random? This way I can replace std::random features by boost::random ones without littering the code by conditional compiles.
  • on the other hand, I must conditionally compile the binder stuff and the random_shuffle/shuffle- dichotomy
Contributor

DanielaE commented Nov 7, 2016

Raffi, I think that is possible, but I need your advise on two points:

  • do you mind taking a dependency on Boost.Random? This way I can replace std::random features by boost::random ones without littering the code by conditional compiles.
  • on the other hand, I must conditionally compile the binder stuff and the random_shuffle/shuffle- dichotomy
@rogeeff

This comment has been minimized.

Show comment
Hide comment
@rogeeff

rogeeff Nov 7, 2016

Contributor

Let's not introduce the dependency on another boost library from the
implementation if it is possible. Can we just guard these changes with
ifdef c++11?

On Mon, Nov 7, 2016 at 1:35 AM, Daniela Engert notifications@github.com
wrote:

Raffi, I think that is possible, but I need your advise on two points:

  • do you mind taking a dependency on Boost.Random? This way I can
    replace std::random features by boost::random ones without littering the
    code by conditional compiles.
  • on the other hand, I must conditionally compile the binder stuff and
    the random_shuffle/shuffle- dichotomy


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#106 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AGNZKZXXblzKC85myAfwTykF8EL9J-7_ks5q7sbPgaJpZM4Kqk6R
.

Gennadiy Rozental

Contributor

rogeeff commented Nov 7, 2016

Let's not introduce the dependency on another boost library from the
implementation if it is possible. Can we just guard these changes with
ifdef c++11?

On Mon, Nov 7, 2016 at 1:35 AM, Daniela Engert notifications@github.com
wrote:

Raffi, I think that is possible, but I need your advise on two points:

  • do you mind taking a dependency on Boost.Random? This way I can
    replace std::random features by boost::random ones without littering the
    code by conditional compiles.
  • on the other hand, I must conditionally compile the binder stuff and
    the random_shuffle/shuffle- dichotomy


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#106 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AGNZKZXXblzKC85myAfwTykF8EL9J-7_ks5q7sbPgaJpZM4Kqk6R
.

Gennadiy Rozental

@DanielaE

This comment has been minimized.

Show comment
Hide comment
@DanielaE

DanielaE Nov 7, 2016

Contributor

Ok, will do when I am back from work this evening. I must look for the appropriate Boost config macro that will guard the changes. Do you have one offhand?

Contributor

DanielaE commented Nov 7, 2016

Ok, will do when I am back from work this evening. I must look for the appropriate Boost config macro that will guard the changes. Do you have one offhand?

@rogeeff

This comment has been minimized.

Show comment
Hide comment
@rogeeff

rogeeff Nov 7, 2016

Contributor

Can you check with boost dev mailing list? Maybe boost.config developers
can chime in.

On Mon, Nov 7, 2016 at 2:13 PM, Daniela Engert notifications@github.com
wrote:

This is the updated patch. But there is a serious drawback and I have no
idea how to get around that: there ist no BOOST_ feature test macro which
tells me if std::shuffle is available in or not. I tried to
infer it from the presence of but this doesn't work f.e. with
vc10. both vc12 and vc14 are fine. Unless someone has a good idea how to
get around that problem I'm stuck.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#106 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AGNZKQO2TUCm_UVPyDbCcscUy2xaGTsYks5q73hVgaJpZM4Kqk6R
.

Gennadiy Rozental

Contributor

rogeeff commented Nov 7, 2016

Can you check with boost dev mailing list? Maybe boost.config developers
can chime in.

On Mon, Nov 7, 2016 at 2:13 PM, Daniela Engert notifications@github.com
wrote:

This is the updated patch. But there is a serious drawback and I have no
idea how to get around that: there ist no BOOST_ feature test macro which
tells me if std::shuffle is available in or not. I tried to
infer it from the presence of but this doesn't work f.e. with
vc10. both vc12 and vc14 are fine. Unless someone has a good idea how to
get around that problem I'm stuck.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#106 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AGNZKQO2TUCm_UVPyDbCcscUy2xaGTsYks5q73hVgaJpZM4Kqk6R
.

Gennadiy Rozental

@raffienficiaud

This comment has been minimized.

Show comment
Hide comment
@raffienficiaud

raffienficiaud Nov 8, 2016

Member

@DanielaE thanks for your work. The shuffle issue is currently under discussion on the boost ML. I will rework your patch a bit, but I have to say I will hardly make it for 1.63.

Member

raffienficiaud commented Nov 8, 2016

@DanielaE thanks for your work. The shuffle issue is currently under discussion on the boost ML. I will rework your patch a bit, but I have to say I will hardly make it for 1.63.

@DanielaE

This comment has been minimized.

Show comment
Hide comment
@DanielaE

DanielaE Nov 8, 2016

Contributor

Hi Raffi,

I will try to pick up the latest discussions on the Boost ML. I am far too busy with my day job and real life to follow in real time. Thanks for looking into this and I am looking forward to your modifications. I never expected it to make it for 1.63 because of timing and the current state of affairs w.r.t. shuffle, though.

Contributor

DanielaE commented Nov 8, 2016

Hi Raffi,

I will try to pick up the latest discussions on the Boost ML. I am far too busy with my day job and real life to follow in real time. Thanks for looking into this and I am looking forward to your modifications. I never expected it to make it for 1.63 because of timing and the current state of affairs w.r.t. shuffle, though.

@raffienficiaud

This comment has been minimized.

Show comment
Hide comment
@raffienficiaud

raffienficiaud Nov 8, 2016

Member

@DanielaE I will have a look, no worries, and cool that you do not expect it for 1.63 :)

Member

raffienficiaud commented Nov 8, 2016

@DanielaE I will have a look, no worries, and cool that you do not expect it for 1.63 :)

@raffienficiaud

This comment has been minimized.

Show comment
Hide comment
@raffienficiaud

raffienficiaud Jan 30, 2017

Member

I still did not have the time to setup a new build agent with the latest VS. Will work on that for 1.64

Member

raffienficiaud commented Jan 30, 2017

I still did not have the time to setup a new build agent with the latest VS. Will work on that for 1.64

@raffienficiaud

This comment has been minimized.

Show comment
Hide comment
@raffienficiaud

raffienficiaud Mar 18, 2017

Member

Hi there,

This topic is still open. The latest devs. on the boost ML show that the VS2017 release is working as expected. http://www.boost.org/development/tests/develop/developer/test.html : column 14.10.

It would be kind of you if you can test with the VS2017 official release.

Thanks!

Member

raffienficiaud commented Mar 18, 2017

Hi there,

This topic is still open. The latest devs. on the boost ML show that the VS2017 release is working as expected. http://www.boost.org/development/tests/develop/developer/test.html : column 14.10.

It would be kind of you if you can test with the VS2017 official release.

Thanks!

@raffienficiaud

This comment has been minimized.

Show comment
Hide comment
@raffienficiaud

raffienficiaud Mar 22, 2017

Member

Thanks @DanielaE for trying out Boost.Test with VS2017.
Things seem to work fine and this PR does not seem to be needed anymore. Would you mind if I close it?

Member

raffienficiaud commented Mar 22, 2017

Thanks @DanielaE for trying out Boost.Test with VS2017.
Things seem to work fine and this PR does not seem to be needed anymore. Would you mind if I close it?

@MarcelRaad

This comment has been minimized.

Show comment
Hide comment
@MarcelRaad

MarcelRaad Mar 22, 2017

Contributor

@raffienficiaud It's needed for VS2017 when compiling in C++17 mode (the default is C++14).

Contributor

MarcelRaad commented Mar 22, 2017

@raffienficiaud It's needed for VS2017 when compiling in C++17 mode (the default is C++14).

@raffienficiaud

This comment has been minimized.

Show comment
Hide comment
@raffienficiaud

raffienficiaud Mar 22, 2017

Member

@MarcelRaad good to know! So let's leave this open then.

Member

raffienficiaud commented Mar 22, 2017

@MarcelRaad good to know! So let's leave this open then.

@DanielaE

This comment has been minimized.

Show comment
Hide comment
@DanielaE

DanielaE Mar 22, 2017

Contributor

@raffienficiaud these changes are still required for compilation with /std:c++latest. I've run the test-suite with this flag enabled on my branch my-1.64.0 and without this flag on the master branch of Boost.Test.

Contributor

DanielaE commented Mar 22, 2017

@raffienficiaud these changes are still required for compilation with /std:c++latest. I've run the test-suite with this flag enabled on my branch my-1.64.0 and without this flag on the master branch of Boost.Test.

Conditionally replace deprecated/removed C++98 binders, adapters, and…
… random_shuffle by emulations using more modern equivalents.

Signed-off-by: Daniela Engert <dani@ngrt.de>
@DanielaE

This comment has been minimized.

Show comment
Hide comment
@DanielaE

DanielaE May 1, 2017

Contributor

There are Boost configuration macros regarding removed C++98 library features available now. I've reworked my patch to take advantage of it. At present, these macros are defined on msvc 14.0 and 14.1 in C++17 mode only, so far. Therefore the modified code paths trigger only in these cases.

Successfully tested on msvc 9.0, 10.0, 12.0, 14.0 and 14.1

Contributor

DanielaE commented May 1, 2017

There are Boost configuration macros regarding removed C++98 library features available now. I've reworked my patch to take advantage of it. At present, these macros are defined on msvc 14.0 and 14.1 in C++17 mode only, so far. Therefore the modified code paths trigger only in these cases.

Successfully tested on msvc 9.0, 10.0, 12.0, 14.0 and 14.1

@raffienficiaud

This comment has been minimized.

Show comment
Hide comment
@raffienficiaud

raffienficiaud May 15, 2017

Member

Thanks again all of you for your patience, I finally have VS2017 running on my build agents and hopefully understood how to force the C++17 flags with b2.
Pulled to the branch origin/topic/PR106-VS2017-C++17-compatibility, under testing.

Will post here the result pretty soon.

Member

raffienficiaud commented May 15, 2017

Thanks again all of you for your patience, I finally have VS2017 running on my build agents and hopefully understood how to force the C++17 flags with b2.
Pulled to the branch origin/topic/PR106-VS2017-C++17-compatibility, under testing.

Will post here the result pretty soon.

@raffienficiaud raffienficiaud merged commit f3a26e0 into boostorg:develop May 22, 2017

@raffienficiaud

This comment has been minimized.

Show comment
Hide comment
@raffienficiaud

raffienficiaud May 23, 2017

Member

In develop

Member

raffienficiaud commented May 23, 2017

In develop

@DanielaE

This comment has been minimized.

Show comment
Hide comment
@DanielaE

DanielaE May 23, 2017

Contributor

Thanks, Raffi!

Contributor

DanielaE commented May 23, 2017

Thanks, Raffi!

@jzmaddock

This comment has been minimized.

Show comment
Hide comment
@jzmaddock

jzmaddock Jun 14, 2017

Contributor

Can this be merged to master please? I'd like to merge multiprecision to master but can't without this.

Many thanks!

Contributor

jzmaddock commented on f3a26e0 Jun 14, 2017

Can this be merged to master please? I'd like to merge multiprecision to master but can't without this.

Many thanks!

This comment has been minimized.

Show comment
Hide comment
@raffienficiaud

raffienficiaud Jun 14, 2017

Member

It is on develop but I was waiting for the availability of a VS C++17 runner to proceed further. Maybe you can participate to the discussion on the boost testing ML?
I will merge asap, nothing seems to have broken for the other runners.

Member

raffienficiaud replied Jun 14, 2017

It is on develop but I was waiting for the availability of a VS C++17 runner to proceed further. Maybe you can participate to the discussion on the boost testing ML?
I will merge asap, nothing seems to have broken for the other runners.

This comment has been minimized.

Show comment
Hide comment
@jzmaddock

jzmaddock Jun 14, 2017

Contributor
Contributor

jzmaddock replied Jun 14, 2017

This comment has been minimized.

Show comment
Hide comment
@raffienficiaud

raffienficiaud Jun 14, 2017

Member

I have my own private CI as well, but the status is missing from the official boost regression board, and I was hoping to have that prior to merging to master. My CI is passing anyway, so the risk of merging is low.

Member

raffienficiaud replied Jun 14, 2017

I have my own private CI as well, but the status is missing from the official boost regression board, and I was hoping to have that prior to merging to master. My CI is passing anyway, so the risk of merging is low.

This comment has been minimized.

Show comment
Hide comment
@malaggan

malaggan Jul 22, 2017

Boost.Test is completely unusable, without this commit, on recent clang (since at least a50011; released 22 hours ago as of this writing) when using -std=c++1z. The functions std::random_shuffle and std::bind1st are removed and the Boost.Test include files fail to compile; complaining about their absence. It would help if this commit was on master so package managers which can be instructed to fetch from it (like homebrew's "--HEAD" option) can solve this problem without having to manually download and compile the develop branch.

P.S.: I am not sure if I should have posted this instead as an bug report, since technically the solution is already in the repo. If I should have posted it as a bug report, kindly let me know and I will.

Thank you.

malaggan replied Jul 22, 2017

Boost.Test is completely unusable, without this commit, on recent clang (since at least a50011; released 22 hours ago as of this writing) when using -std=c++1z. The functions std::random_shuffle and std::bind1st are removed and the Boost.Test include files fail to compile; complaining about their absence. It would help if this commit was on master so package managers which can be instructed to fetch from it (like homebrew's "--HEAD" option) can solve this problem without having to manually download and compile the develop branch.

P.S.: I am not sure if I should have posted this instead as an bug report, since technically the solution is already in the repo. If I should have posted it as a bug report, kindly let me know and I will.

Thank you.

This comment has been minimized.

Show comment
Hide comment
@raffienficiaud

raffienficiaud Jul 23, 2017

Member

@malaggan this is already on master and should be part of boost 1.65 rc1. Have you actually tested?

Member

raffienficiaud replied Jul 23, 2017

@malaggan this is already on master and should be part of boost 1.65 rc1. Have you actually tested?

This comment has been minimized.

Show comment
Hide comment
@malaggan

malaggan Jul 23, 2017

@raffienficiaud You're right, my bad. It was the end of a long day and it appeared from the comments here that the commit was not merged into master yet. I apologize for wasting your time.

malaggan replied Jul 23, 2017

@raffienficiaud You're right, my bad. It was the end of a long day and it appeared from the comments here that the commit was not merged into master yet. I apologize for wasting your time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment