Suggestion for improvement: ofxThread.cpp

Hi there!

I’m pretty new to C++ and the openFrameworks project and to deepen my knowledge I just read the ofxThread.cpp file in of_preRelease_v0.05_xcode_FAT/addons/ofxThread/src.
Now I wonder if the constructor ofxThread::ofxThread() and the ofxThread::isThreadRunning() method could be improved in some way?

ofxThread::ofxThread() old version:

	threadRunning = false;  

my draft proposal:

ofxThread::ofxThread() : threadRunning(false) { }  

Usage of initialization lists. Memory allocation and initialization in one step: slight speed benefits.

ofxThread::isThreadRunning() old version

bool ofxThread::isThreadRunning(){  
	//should be thread safe - no writing of vars here  
	if(threadRunning) return true;  
	else return false;  

my draft proposal:

inline bool ofxThread::isThreadRunning()  
   return this->threadRunning;  

Removed unnecessary ‘if’ and ‘else’ statement. Inline and fewer statements provide more speed.

If I’m wrong I’d be glad if you could point out the mistake I made.
Thank you in advance, johannesh.

nice! and welcome to the forum !

I didn’t write ofxThread, but I think it’s nice to see people throwing up changes on the forum – and discussing how to improve code.

Your changes bring up the trade off between legibility and speed, and we’ve often times tried to err on the side of making legible code that’s easy to read.

Personally, we’ve shied away from initialization lists just because they are strange for non c++ programmers to look at and we rarely have objects that are being allocated or deallocated too often. it would make sense, if you have to allocate thousands per frame, for example, so that the small savings of using an initialization list would add up to be meaningful, but currently I’m just not sure that that’s a worthwhile change since I don’t see alot of ofxThread allocation happening, and I can imagine non c++ programmers getting confused with the code (however, didi on the forums made a good argument that we should be teaching people these things).

the other change makes alot of sense to me because you would call it once per frame and it could make a difference.

thanks again!!

ps : if you are up for optimizing, there are some parts of OF that can use extra eyes – ofImage and ofVideoPlayer (esp, the quicktime code). Both have code for pixel conversion, are heavily used by the community, and I think both can be made alot faster with different operations. I’ll try to jot some notes in the next days about what can be improved.

Thank you for your fast reply.
I’m looking forward to read and learn from the ofImage and ofVideoPlayer in the next days and maybe I can point out some possible improvements : )
Could you send me a private message or reply to this thread when you’ve finished your notes?
Thanks, johannesh.