Code review comment for ~epics-core/epics-base/+git/Com:thread-join

Revision history for this message
mdavidsaver (mdavidsaver) wrote :

> I noticed the other day that VxWorks 6.9 added this API:

8-()

> Should the epicsThreadJoin() API take a timeout argument?

I would say no. Given the nature of threads, join is only useful if the caller has already done something to ensure the thread will (eventually) return. If there is some doubt about this, then it should be handled at this earlier phase.

Otherwise it seems a bit like allowing free() to timeout.

> Shouldn't the C++ class also have a join() method?

I suppose this could be added. I was thinking first of a non-invasive change.

https://en.cppreference.com/w/cpp/thread/thread/join

> EPICS_THREAD_CAN_JOIN

This will go away now.

> epicsThreadJoin(epicsThreadGetIdSelf()) should become unjoinable

I'll take a look.

> adding some tests to epicsThreadTest.cpp would be helpful

Any thoughts on what to test? I couldn't think of any meaningful unittest to see if resources have actually been freed. Mostly I added prints while developing (especially for RTEMS), and ran epicsThreadTest through valgrind on Linux.

« Back to merge proposal