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 with C++11 [DISCL-391] #130

Merged
merged 1 commit into from
Sep 28, 2016
Merged

Conversation

rdumusc
Copy link

@rdumusc rdumusc commented Sep 26, 2016

Notes:

  • FramePtr must remain a boost::shared_ptr as long as Tide is
    serializing it and does not mandate boost >= 1.56.
  • boost::signals2 for Stream::disconnected is hard to replace,
    however it is only used internally by QmlStreamerImpl.cpp.
  • program_options and unit_test could be optional.

Copy link
Contributor

@tribal-tec tribal-tec left a comment

Choose a reason for hiding this comment

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

Hmm, not removing boost entirely makes a bit wonder how useful that change is. But +1.

@@ -132,7 +126,7 @@ class Stream
/** @name Asynchronous send API */
//@{
/** Future signaling success of asyncSend(). @version 1.1 */
Copy link
Contributor

Choose a reason for hiding this comment

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

That breaks API imo. No longer version 1.1

@rdumusc
Copy link
Author

rdumusc commented Sep 27, 2016

Yes, I thought I could easily remove all when I started. But I couldn't find an elegant solution for the shared_ptr serialization. I could copy the Frame in Tide from std::shared_ptr to boost::shared_ptr before serializing, which wouldn't be too expensive because the segments are QByteArray (implicitly shared) but still look a bit awkward. The boost::signal could be replaced by a callback function which would be fine for our use case. I was not sure and decided to stop here, which is still a step in the right direction (the only problem is the one API breakage you pointed out).

@rdumusc
Copy link
Author

rdumusc commented Sep 27, 2016

And I notice now that the API change requires a change in Equalizer because eq/deflect/Proxy.cpp because it explicitly uses boost in a make_ready_future() function...

@rdumusc
Copy link
Author

rdumusc commented Sep 27, 2016

Updated: I decided that since this was already breaking the API I might as well do all the changes and make boost optional.

@rdumusc rdumusc changed the title Replace boost with C++11 where applicable [DISCL-391] Replace boost with C++11 [DISCL-391] Sep 27, 2016
@@ -17,15 +17,13 @@ set(DEFLECT_LICENSE BSD)
set(DEFLECT_DEPENDENT_LIBRARIES Boost)
Copy link
Contributor

Choose a reason for hiding this comment

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

can be removed now?

Copy link
Author

Choose a reason for hiding this comment

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

done

@@ -5,6 +5,9 @@ Changelog {#Changelog}

### 0.12.0 (git master)

* [130](https://github.com/BlueBrain/Deflect/pull/130)
Replaced boost by C++11. Boost is now an optional dependency and it is used
only by the tests. Minor API changes were introdued by this change.
Copy link
Contributor

Choose a reason for hiding this comment

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

Not so minor imo.
typo 'introdued'

Copy link
Author

Choose a reason for hiding this comment

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

done, replaced minor by some

boost program_options and unit_test are now optional (tests only)
@tribal-tec
Copy link
Contributor

+1

@rdumusc rdumusc merged commit 07711c1 into BlueBrain:master Sep 28, 2016
@rdumusc rdumusc deleted the cpp11 branch September 28, 2016 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants