Code review comment for lp:~ralph-lange/epics-base/thread-hooks

Revision history for this message
Ralph Lange (ralph-lange) wrote :

Hi Andrew,

> The name 'epicsShowThreadInfo' implies this routine is part of the epicsShow
> subsystem. The original name 'showThreadInfo' was static and thus only used
> internally to the posix implementation, but if you're making it visible I
> think it needs to match the other epicsThread... names. Maybe name it
> epicsThreadShowPrivate(), which matches the Win32 implementation (although
> there the routine is static).

Will do.
On a closely related issue, I am quite unhappy with the confusing names of the public API functions

epicsThreadPrivateCreate
epicsThreadPrivateDelete
epicsThreadPrivateSet
epicsThreadPrivateGet

that are - unlike the other funtions that contain "Private" - *not* private, but public functions.
Also, they implement what is more commonly called "thread local storage".
Do you think it would be worthwhile renaming them to *Local* and deprecate the existing names, adding macros to continue supporting them?!

> If you don't intend that routine to be called by user code the declaration for
> it doesn't have to appear in any header files either, it could just appear in
> the posix/osdThread.c file where it's used.

True. Will do.

> I would also prefer that it take
> an epicsThreadId argument instead of the epicsThreadOSD* pointer; that should
> remain for internal use only (epicsThread.h has a typedef making the two
> identical).

If (after the mentioned change) the function is private, not in any header file, and any implementation would have to cast the argument to an epicsThreadOSD* pointer anyway, I don't see the point in using the opaque type.

~Ralph

« Back to merge proposal