Let’s review some code: C++ #2

Welcome to the next pikoTutorial !

Below you can find the code that we will be reviewing today. Before you scroll down to section where I fix it, feel free to check how many mistakes you can find.

#include <iostream>

class Logger
{
public:
    ~Logger()
    {
        std::cout << "Logger destroyed" << std::endl;
    }
    virtual void LogInfo(const char* message) = 0;
};

class ConsoleLogger : public Logger
{
public:
    void LogInfo(const char* message) override
    {
        std::cout << "[Console][Info] " << message << std::endl;
    }

    ~ConsoleLogger() {
        std::cout << "ConsoleLogger destroyed" << std::endl;
    }
};

const char* CreateInfoMessage()
{
    const std::string message_string = "Some fixed message";
    return message_string.c_str();
}

int main() {
    Logger* logger = new ConsoleLogger();
    const char* message = CreateInfoMessage();

    logger->LogInfo(message);

    delete logger;

    return 0;
}

Without running this code, one could expect that the output which it generates looks like this:

[Console][Info] Some fixed message
ConsoleLogger destroyed
Logger destroyed

But when we run it, the actual output is:

[Console][Info] �

Let's start with the first issue visible in the output - why do we see some trash instead of "Some fixed message"? The root cause lies in the CreateInfoMessage function:

const char* CreateInfoMessage()
{
    const std::string message_string = "Some fixed message";
    return message_string.c_str();
}

Take a deeper look what is this function doing:

• it creates a local string variable message_string
• it returns the underlying const char* string using c_str() function
• it destroys the local variable message_string

Remember that std::string is an owning type, so when it gets destroyed, the underlying resource it manages is also destroyed. This means that the thing this function returns is destroyed directly after the function exits, giving no chance any client to use it.

There's also another problem here - the usage of const char* may be considered as obsolete, so for the new code it's much better to use its modern version which is std::string_view. Let's then change our example so that it uses std::string_view instead of const char*. Using std::string_view does not resolve lifetime issues, but brings number of other benefits over using const char*.

But coming back to the core of our problem - to fix the issue related to printing trash instead of actual message, we could return a string literal from CreateInfoMessage():

std::string_view CreateInfoMessage()
{
    return "Some fixed message";
}

However, if this is hardcoded anyway, let's just define it as a static constant string literal known at compile time using constexpr. This way, CreateInfoMessage() becomes unnecessary at all.

static constexpr std::string_view kFixedInfoMessage {"Some fixed message"};

Now the main function is simplified:

int main() {
    Logger* logger = new ConsoleLogger();

    logger->LogInfo(kFixedInfoMessage);

    return 0;
}

And we finally see the correct string in the output:

[Console][Info] Some fixed message

Let’s then take a look at the second issue: why don’t we see the expected logs from the destructors of both classes? The answer is simple - we don't see them because our ConsoleLogger class instance is never destroyed. There is no delete associated with the new operator, so this memory has been leaked.

To remove the memory leak, we can use a smart pointer which handles the memory deallocation automatically on scope exit:

std::unique_ptr<Logger> logger = std::make_unique<ConsoleLogger>();

However, although the memory leak is no longer there, this solves the problem only partially because now in the program output we see the log only from the Logger's class destructor.

[Console][Info] Some fixed message
Logger destroyed

Let's then take a look at the Logger base class (it already contains changes from the previous parts):

class Logger
{
public:
    ~Logger()
    {
        std::cout << "Logger destroyed" << std::endl;
    }
    virtual void LogInfo(const std::string_view message) = 0;
};

We see that it contains a pure virtual method, so this class is definitely intended to be derived from, but at the same time its destructor is not marked as virtual. Virtual destructor is very important because it ensures that the correct destructor is called also for derived objects when they are deleted through a pointer to the base class. This is essential to avoid resource leaks and undefined behavior in C++.

This is exactly the case we have in our example code because we create and delete a derived object (ConsoleLogger) through a pointer to the base class (Logger):

Logger* logger = new ConsoleLogger();

In the output, we see the Logger destroyed line, but no ConsoleLogger destroyed what means that the destructor of ConsoleLogger has never been called, so this resource has been leaked. After adding virtual keyword to the Loggers's destructor, the output is finally fully correct:

[Console][Info] Some fixed message
ConsoleLogger destroyed
Logger destroyed

Eventually, we end up with the whole code looking like this:

#include <iostream>
#include <memory>

static constexpr std::string_view kFixedInfoMessage {"Some fixed message"};

class Logger
{
public:
    virtual ~Logger()
    {
        std::cout << "Logger destroyed" << std::endl;
    }
    virtual void LogInfo(const std::string_view message) = 0;
};

class ConsoleLogger : public Logger
{
public:
    void LogInfo(const std::string_view message) override
    {
        std::cout << "[Console][Info] " << message << std::endl;
    }

    ~ConsoleLogger() {
        std::cout << "ConsoleLogger destroyed" << std::endl;
    }
};

int main() {
    std::unique_ptr<Logger> logger = std::make_unique<ConsoleLogger>();

    logger->LogInfo(kFixedInfoMessage);

    return 0;
}