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

Cleanup singleton #1685

Merged
merged 6 commits into from
May 16, 2018
Merged

Cleanup singleton #1685

merged 6 commits into from
May 16, 2018

Conversation

Nicogene
Copy link
Member

@Nicogene Nicogene commented May 10, 2018

This PR c++11-fy all(?) the singletons in yarp.

Please review code.

@Nicogene Nicogene self-assigned this May 10, 2018
@Nicogene Nicogene requested a review from drdanz May 10, 2018 12:21
@Nicogene Nicogene mentioned this pull request May 10, 2018
8 tasks
@Nicogene Nicogene force-pushed the cleanupSingleton branch from 8438b13 to 66b4179 Compare May 11, 2018 16:42
@@ -123,12 +123,17 @@ New Features
[C++11 Mutex concept](http://en.cppreference.com/w/cpp/concept/Mutex).
* Added a Timer class to execute a static or member function in a separate
thread after a period x, every y milliseconds, for z times or w seconds.
* `yarp::os::Carriers` `yarp::os::LogForwarder` singleton c++11-fied.
Copy link
Member

Choose a reason for hiding this comment

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

I know I'm usually annoying on this, but this is the kind of changes that has no impact at all on the user, therefore I believe it would be better not to mention it on the changelog...

{
yarp::os::LogForwarder::clearInstance();
}

Copy link
Member

Choose a reason for hiding this comment

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

yarpCustomFini() has been there since the origin of YARP, probably for a symmetry reason, see

extern "C" void yarpCustomFini() {
}

Also the LogForwarder should have never been initialized there, the right place would be finiMinimum, this call is supposed to be there for initializing stuff initialized in yarpCustomInit.

Anyway, I'd rather just remove the yarp::os::LogForwarder::clearInstance(); call for now and restore the original empty body of the call (the compiler will probably optimize it anyway...)

@Nicogene Nicogene force-pushed the cleanupSingleton branch from 66b4179 to 7723709 Compare May 14, 2018 17:08
@Nicogene Nicogene added PR Status: Changelog - Not Required This PR does not need a changelog entry and removed PR Status: Changelog - OK This PR has a proper changelog entry labels May 14, 2018
@Nicogene
Copy link
Member Author

👍 done

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.

2 participants