Ytdyklly

I have an older class that I am working on and I'm updating it to remove "Windows" specific code. It had some "Thread Protection Code" that used CRITICAL_SECTION. I'm in the process of changing the use of CRITICAL_SECTION to use std::mutex and std::lock_guard. One thing to take note of is that my class does have static functions and this class is of a singleton type.

This is what my old code would have looked like.

-Logger.h-

#pragma once



#include "Singleton.h"



class Logger final : public Singleton {

public: 

    // enum here



private:

    // private variables not of concern



    CRITICAL_SECTION criticalSection_;



public:

    explicit Logger( const std::string& filename );

    virtual ~Logger();



    static void log( const std::string& text, LoggerType type );

    static void log( const std::ostringstream& stream, LoggerType type );

    static void log( const char* text, LoggerType type )

};

-Logger.cpp-

#include "Logger.h"



#include "BlockThread.h" // old class that would block threads - replacing with mutex lock_guard



#include "TextFileWriter.h" // file handler the Logger uses to write text files



// since this is a singleton type class as I only want a single logger per

// application run; here I use a static pointer to this class, the reason

// the log functions are static to begin with.

static Logger* spLogger_ = nullptr;



Logger::Logger( const std::string& filename ) :

Singleton( LOGGER ) {  // Base class takes an enum type of Singleton

    // variable initializations

    // ...

    // ------------------------



    InitializeCriticalSection( &criticalSection_ );

    BlockThread blockThread( criticalSection ); // Enter Critical Section



    // Start the log file

    TextFileWriter file( filename_, false, false ); //



    spLogger_ = this;

}



Logger::~Logger() {

    spLogger_ = nullptr;



    DeleteCriticalSection( &criticalSection_ );

}



void Logger::log( const std::string& text, LoggerType type ) {

    log( text.c_str(), type );

}



void Logger::log( const std::ostringstream& stream, LoggerType type ) {

    log( stream.str().c_str(), type );

}



void Logger::log( const char* text, LoggerType type ) {

    if ( nullptr = spLogger_ ) {

        // output error message

        return; 

    }



    BlockThread blockThread( spLogger->criticalSection_ ); // Enter Critical Section



    std::ostringstream stream;



    // setup logger's text formatting 



    // get the date and time 



    // push the date and time into the stream



    // push the text message into the stream



    // print stream to console



    try {

        TextFileWriter file (spLogger_->filename_, true, false );

        file.write( stream.str() );

    } catch( ... ) {

        // output error message failed to write to file.

    }

}

If you need to see the BlockThread class I'll show it here just incase...

-BlockThread.h-

#pragma once



class BlockThread final {

private:

    CRITICAL_SECTION* criticalSection_;



public:

    explicit BlockThread( CRITICAL_SECTION& criticalSection );

    ~BlockThread();

};

-BlockThread.cpp-

#include "BlockThread.h"



BlockThread::BlockThread( CRITICAL_SECTION& criticalSection ) {

    criticalSection_ = &criticalSection;

    EnterCriticalSection( criticalSection_ );

}



BlockThread::~BlockThread() {

    LeaveCriticalSection( criticalSection_ );

}

This was how my old classes were set up.

This is my attempt to remove BlockThread class and the CRITICAL_SECTION and its related functions by replacing them with std::mutex and std::lock_guard

-Logger.h-

#pragma once



#include "Singleton.h"



#include <mutex> // added this for mutex



class Logger final : public Singleton {

public:

    // public enum



private:

    // private members



    // CRITICAL_SECTION

    static std::mutex critical_;



public:

    // constructor, destructor and log function declarations same as above



};

-Logger.cpp-

#include "Logger.h"



static Logger* spLogger_ = nullptr;

std::mutex Logger::critical_{};



Logger::Logger( const std::string& filename ) {

    // init variables



    // InitializeCriticalSection( &criticalSection_ )

    // BlockThread blockThread( criticalSection_ );

    std::lock_guard<std::mutex> lock(critical_ );



    TextFileWriter file( filename, false, false );



    spLogger_ = this;

}



Logger::~Logger() {

    spLogger_ = nullptr;



    // DeleteCriticalSection( &criticalSection_ );



    // Left empty don't think I have to do anything since `lock_guard`

    // is destroyed once it leaves its scope...

}



// skip the first two log functions no difference here...



void Logger::log( const char* text, LoggerType type ) {

    // check if spLogger_ is null if so print error message & return



    // BlockThread blockThread( spLogger_->criticalSection_ ); // Enter Critical Section



    std::lock_guard<std::mutex> lock( spLogger_->criticalSection_ );



    // text formatting, date-time and message



    // print stream to console



    // try catch block same as above.

}

There's a set of questions that are sort of related; that's why I feel they are all important in this situation...

What I would like to know:

• Is the replacement of my original CRITICAL_SECTION it's related functions and my original BlockThread class with std::mutex and std::lock_guard correct?


• Will this behave in the same manner or fashion?


• If not what would be the correct way to replace them as I'm not real familiar with std::mutex & std::lock_guard? - I'm trying to get a better grasp and understanding of how to use them properly.


• Is there anything that I am missing, any corner cases or code smell?


• 
  -Note- I'm not really concerned with the commented out code as I know the original class works fine; I'm mainly concerned where I made the replacements.


• What can I do to make this code follow modern C++ standards; to make it portable, cross-platform capable etc.

-Edit-

One of the major motivations for writing this question is that I have other classes in a decent size library that had previously used CRITICAL_SECTION its related functions and my old BlockThread class. Once I know how to properly replace them with std::mutex, std::lock_guard or any of their variations... Then I should easily be able to replace all thread safety features in my library.