1
\$\begingroup\$

I have inherited a C++ Win32 thread class which roughly works like this. I am looking for ways to improve the design; specifically I am looking for a way to get rid of the spurious Resume() call at the end of initialization. Is there a way this can be automated or omitted?

Base class

class CThread {
public:
    CThread(DWORD dwCreationFlags = 0)
        : m_hThread(Create(ThreadProc, 
                           this, 
                           dwCreationFlags, 
                           NULL, 
                           0, 
                           &m_dwThreadId))
    {}

    DWORD Resume() { return ::ResumeThread(m_hThread); }


protected:
    HANDLE  m_hThread;
    DWORD   m_dwThreadId;

private:
    static HANDLE Create(LPTHREAD_START_ROUTINE pThreadProc,
                         LPVOID pParam = NULL,
                         DWORD dwCreationFlags = 0,
                         LPSECURITY_ATTRIBUTES pSecurityAttr = NULL,
                         DWORD dwStackSize = 0,
                         DWORD *pdwThreadId = NULL)
    {
        /* start thread using _beginthreadex */
        return hThread;
    }

    static DWORD WINAPI ThreadProc(LPVOID p)
    {
        CThread* pThreadObj = reinterpret_cast<CThread *>(p);
        return pThreadObj->Execute();
    }
};

So now when I make a subclass to start the thread, I need to start it suspended because otherwise it might run without being fully initialized, and then resume it after initialization:

Actual thread implementation class

class CCreateRestorePointThread
        : public CThread
{
    CCreateRestorePointThread(std::wstring const&    name,
                              zRestorePointEventType eventType,
                              zRestorePointType      restorePointType)
/* I think I need to start suspended this so that initialization can take place */
        : CThread(CREATE_SUSPENDED)
        , m_hSrClient(::LoadLibraryW(L"srclient.dll"))
        , m_bRet(FALSE)
        , m_isFinished(false)
    {
        /* Initialize Restore Point System, structures etc. */
        this->Resume(); /* I wanna get rid of this line */
    }

private:
    DWORD Execute()
    {
        /* Create Restore Point using class members */
        return m_stateMgrStatus.nStatus;
    }    
};
\$\endgroup\$
3
  • \$\begingroup\$ Please try to make the example compilable. You never declare or define CThread::Execute which is kind of important. In this case I do not trust myself to advice things without trying them first. \$\endgroup\$
    – nwp
    Nov 13 '14 at 19:42
  • \$\begingroup\$ C++ now has threads built-in. You should probably be using the standard library version. It will call the platform/OS specific version behind the Scenes. \$\endgroup\$ Nov 13 '14 at 21:55
  • \$\begingroup\$ It is somewhat difficult to review this. Without providing further details, such as the purpose of this class and an example driver, I can only say that this is probably a bad design. I see at least two resources being managed (Thread handle and DLL hmodule). I do not see any RAII or cleaning up of these objects. The DLL seems to be loaded in the calling space rather than the thread space. I'm not sure whether that matters or not. I'm guessing Execute is a virtual function, but there's no way to know. In summary, please provide more context. \$\endgroup\$
    – jliv902
    Nov 14 '14 at 19:27
2
\$\begingroup\$

Big Bad Issue:

The callback function for threads libraries are usually a C-function pointer. As such the thread library depends on the callback having a C ABI (not a C++ ABI). This means the function you pass into the threading API must be declared:

extern "C" void* XXXX(void*); // Assuming return and take void* parameters.
                              // Which is basically all you can do with a C API.

Note the extern "C" must be there for it to be a valid C++ program.

You are using a static member function as your callback.

static DWORD WINAPI ThreadProc(LPVOID p)

It may look like it is working but this is just pure coincidence that the ABI for static member function happens to be the same on your platform as the ABI for C functions (there is no requirement in the standard). Nor do any OS that I know give guarantees that they will not change this ABI.

You should declare it like this:

// I also suspect the you are using windows
// They also have some special macro magic you may want to look at to force
// specific calling conventions.:
//
// __cdecl    =>  One of those magic double underscore reserved identifiers.
// http://msdn.microsoft.com/en-us/library/kdzttdcb.aspx
//
extern "C" DWORD WINAPI ThreadProc(LPVOID p)
{
    CThread* pThreadObj = reinterpret_cast<CThread *>(p);
    return pThreadObj->Execute();
}

class CCreateRestorePointThread: public CThread
{
    friend DWORD WINAPI ThreadProc(LPVOID p);
    ...
};
\$\endgroup\$
3
  • 1
    \$\begingroup\$ Thanks for your input, but this is not an issue here: My callback function is declared WINAPI which means __stdcall and beginthreadex expects exactly that calling convention. \$\endgroup\$ Nov 14 '14 at 15:54
  • \$\begingroup\$ @FelixDombek: Maybe. I still see this as a bad habit as the way you are doing it will not translate well to other platforms were you have less control over defining the available calling conventions. Bad habits are the worst as you apply them automatically without thinking even when the underlying context changes. \$\endgroup\$ Nov 15 '14 at 1:03
  • 1
    \$\begingroup\$ PS. You should still be using std::thread \$\endgroup\$ Nov 15 '14 at 1:04

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.