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

Small fixes for windows #558

Merged
merged 9 commits into from
Dec 10, 2014

Conversation

nightrune
Copy link
Contributor

Windows now builds after nomis52's changes.

@peternewman
Copy link
Member

@nightrune you've broken the Linux build, can you fix that so we can merge. May be time for another ifdef...

@@ -46,7 +46,7 @@ bool SetSchedParam(pthread_t thread, int policy,
int r = pthread_setschedparam(thread, policy, &param);
if (r != 0) {
OLA_FATAL << "Unable to set thread scheduling parameters for thread "
<< thread << ": " << strerror(r);
<< thread.x << ": " << strerror(r);
Copy link
Member

Choose a reason for hiding this comment

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

On Linux it appears to be ( http://refspecs.linuxbase.org/LSB_3.1.1/LSB-Core-generic/LSB-Core-generic/libpthread-ddefs.html ):
typedef unsigned long int pthread_t;

So we'll need another ifdef.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, looking at this, on Windows don't you want .p instead, based on the comment? http://pimprenelle.lps.ens.fr/xvindoc/pthread_8h_source.html#l00566

Copy link
Member

Choose a reason for hiding this comment

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

What we want is something like

inline std::ostream& operator<< (std::ostream &out, pthread_t thread) {
#ifdef _WIN32
return out << thread.x
#else
return out << thread
}

in the Thread.h file .

* @return stream
*/
inline std::ostream& operator<<(std::ostream &out, pthread_t thread) {
#ifdef WIN32
Copy link
Member

Choose a reason for hiding this comment

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

_WIN32 not WIN32

@peternewman peternewman added this to the 0.9.4 milestone Dec 7, 2014
@peternewman
Copy link
Member

I think this should be in https://github.com/OpenLightingProject/ola/milestones/0.9.4 so the Windows build doesn't ship in a more broken state, it's functionality should only get better going forwards.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) when pulling 3878ff8 on nightrune:sms-windows-fixes into fd19c2c on OpenLightingProject:master.

@peternewman
Copy link
Member

As an alternative out there thought, what if we created a wrapper class around pthread, which could then implement a to string function?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0%) when pulling 3397f54 on nightrune:sms-windows-fixes into 031a36a on OpenLightingProject:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0%) when pulling 80df266 on nightrune:sms-windows-fixes into 031a36a on OpenLightingProject:master.

@@ -213,8 +217,12 @@ void ThreadTest::testSchedulingOptions() {
OLA_ASSERT_EQ(override_params.priority,
thread.GetSchedulingParams().priority);
}
#else
OLA_INFO << "Scheduling options are not supported on windows..";
Copy link
Member

Choose a reason for hiding this comment

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

I'd make this a warning.

peternewman added a commit that referenced this pull request Dec 10, 2014
@peternewman peternewman merged commit 2779984 into OpenLightingProject:master Dec 10, 2014
@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) when pulling 83d40f1 on nightrune:sms-windows-fixes into 031a36a on OpenLightingProject:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) when pulling 83d40f1 on nightrune:sms-windows-fixes into 031a36a on OpenLightingProject:master.

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.

4 participants