1
\$\begingroup\$

The CSyncObject object in CSingleLock constructor can't be NULL but I want it to work with NULL because I need to use this nest.

BOOL CADODatabase::Open(LPCTSTR lpstrConnection, LPCTSTR lpstrUserID, LPCTSTR lpstrPassword)
{
    CString database = GetSourceDatabase( lpstrConnection, NULL );

    // get the appropriate critical section based on database
    g_dbCriticalSection = GetDbCriticalSection( database );

    if( g_dbCriticalSection) // it can be NULL
    {
        g_dbCriticalSection->Lock(); 
    }

    // more code,  have to unlock() manually in multiple exits!
}

As can be seen, I need to lock() and then unlock() manually at all exit points because its in a nest.

i wrote a class to make CSingleLock work with NULL, my new code looks like this:

BOOL CADODatabase::Open(LPCTSTR lpstrConnection, LPCTSTR lpstrUserID, LPCTSTR lpstrPassword)
{
    CString database = GetSourceDatabase( lpstrConnection, NULL );

    // get the appropriate critical section based on database
    g_dbCriticalSection = GetDbCriticalSection( database );

    CSingleLockEx lock( g_dbCriticalSection, TRUE );

    // eliminates the need to unlock() manually, works like CSingleLock
}

My new class is here:

class CSingleLockEx
{
public:
    CSingleLock* _singleLock;

    CSingleLockEx(CSyncObject* pObject, BOOL bInitialLock = FALSE):
    _singleLock(NULL)
    {
        if (pObject)
            _singleLock = new CSingleLock(pObject, bInitialLock);

    }
    ~CSingleLockEx()
    {
        if (_singleLock)
            delete _singleLock;
    }
};

Since it has to do with critical section, I thought I will put it for review if I have missed anything.

\$\endgroup\$
  • \$\begingroup\$ @AdrianoRepetti no g_dbCriticalSection is pointer to CCriticalSection \$\endgroup\$ – zadane Jul 12 '17 at 18:54
3
\$\begingroup\$

Sure, that seems fine.

  • Obviously you should be using std::unique_ptr<CSingleLock> (and std::make_unique) instead of raw new and delete.

  • If you have access to C++17, Folly, or Boost, consider using std::optional<CSingleLock> to eliminate the heap allocation.

  • If you're not using unique_ptr, you might want to =delete your copy constructor and copy assignment operator (or in C++03, make them private), to ensure that nobody accidentally creates a copy of your object — which if they did would lead to a double-free.


You've also made your member variable _singleLock public, presumably by accident. If you actually want it to be public, consider giving it a more user-friendly name. It's best to keep it private, though.


A simpler approach would be to create a dummy lock, which (assuming either CSingleLock or CMutex is immovable) would actually fix the copyability problem as well.

class CSingleLockEx
{
    CMutex dummy_;
    CSingleLock lk_;
public:
    CSingleLockEx(CSyncObject *obj, bool initialLock = false) :
        lk_((obj ? *obj : dummy_), initialLock)
    {}
};

This way you end up locking something either way (and unlocking something in the destructor as well), but at least when obj==nullptr the "something" you're locking will definitely be uncontended. So I wouldn't expect a noticeable performance hit.

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ Well the GetDbCriticalSection() returns NULL by design (telling not to lock if accessing certain database) so I have to not lock in that case. \$\endgroup\$ – zadane Jul 12 '17 at 20:47
  • \$\begingroup\$ @zadane: You have to not lock the null pointer, of course. But my alternative code doesn't lock a null pointer. It locks the mutex dummy_, which is always perfectly safe and never contended (because it's not public and never visible to anyone else in the program other than this particular CSingleLockEx variable of which it's a non-static member). \$\endgroup\$ – Quuxplusone Jul 12 '17 at 20:50
0
\$\begingroup\$

I think creating a new class for that just obscures the code. To understand your code one has to look up what your class CSingleLockEx makes different from CSingleLock.

The suggestions made by this answer are simple enough that they can be used directly in code. Anyone reading this code will immediately understand it then (provided that they know about the standard classes of course).

Using std::unique_ptr<CSingleLock> (C++11):

std::unique_ptr<CSingleLock> pLock;
if( g_dbCriticalSection ) // it can be NULL
{
    // make_unique is C++14
    pLock = std::make_unique<CSingleLock>( g_dbCriticalSection, TRUE );
    // C++11 alternative:
    // pLock.reset( new CSingleLock( g_dbCriticalSection, TRUE ) );
}

// pLock will unlock automatically when scope ends

Using std::optional<CSingleLock> (C++17, saves a heap allocation and is cleaner code). Alternatively use boost::optional<CSingleLock>.

std::optional<CSingleLock> oLock;
if( g_dbCriticalSection ) // it can be NULL
{
    oLock.emplace( g_dbCriticalSection, TRUE );
}

// oLock will unlock automatically when scope ends
|improve this answer|||||
\$\endgroup\$

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.