5
\$\begingroup\$

I have implemented my own logger based pimpl idiom. As a third party lib, I used spd log. My focus is to provide an interface to the end user who wants to print multiple parameters at once. I also used fold expression and ostringstream to streamline any primitive type of parameter. I would be grateful if you could provide me with your feedback.

//main.cpp
#include <logger.hpp>

int main() {
    logger _logger(logger::trace);
    _logger.LOG(logger::debug, "test1", "test2", 1, 2, 3.1, 'X');
    return 0;
}

//logger.hpp
#pragma once

#include <memory>
#include <sstream>

class logger {
public:
    enum level_enum : int {
        trace = 0,
        debug = 1,
        info = 2,
        warn = 3,
        err = 4,
        critical = 5,
        off = 6,
        n_levels
    };

    explicit logger(logger::level_enum level);
    ~logger();

    template <typename... T>
    void LOG(level_enum level, T... msg) {
        auto stream = dump(std::forward<T>(msg)...);
        print(level, stream);
    }

    template <typename... T>
    std::ostringstream dump(T... msg) {
        std::ostringstream stream;
        ((stream << msg << ' '), ...);
        return stream;
    }

    void print(level_enum level, std::ostringstream& stream);

private:
    class Impl;
    std::unique_ptr<Impl> pImpl;
};

//spd_logger.cpp
#include "spdlog/sinks/basic_file_sink.h"
#include "spdlog/sinks/stdout_color_sinks.h"
#include "spdlog/spdlog.h"
#include <logger.hpp>

#include <memory>

class logger::Impl {
    std::shared_ptr<spdlog::logger> logger;

public:
    explicit Impl(spdlog::level::level_enum level) {
        logger = std::make_shared<spdlog::logger>("LOG");
        logger->sinks().push_back(std::make_shared<spdlog::sinks::stdout_color_sink_mt>());
        logger->sinks().push_back(std::make_shared<spdlog::sinks::basic_file_sink_mt>("logs/basic_sink.txt"));
        logger->set_level(level);
    }

    void log(logger::level_enum level, std::ostringstream& msg) {
        logger->log(static_cast<spdlog::level::level_enum>(level), msg.str());
    }
};

logger::logger(logger::level_enum level)
        : pImpl(std::make_unique<Impl>(static_cast<spdlog::level::level_enum>(level))) {}

logger::~logger() = default;

void logger::print(level_enum level, std::ostringstream& msg) {
    this->pImpl->log(level, msg);
}
\$\endgroup\$
2
  • 1
    \$\begingroup\$ The MOST important thing about loggers; Is that when they are below the alert threshold (ie not logging) that they do zero (or minimal) work. This means that arguments should NOT be evaluated (as they could be expensive function calls). This is where your logger fails. I see that you are trying (with the PIMPL) not to do anything with the parameters when no logging is requried, but all parameters are evalutated and thus can potentially do work. \$\endgroup\$ Commented Nov 19, 2024 at 21:45
  • 1
    \$\begingroup\$ Please do not edit the question, especially the code, after an answer has been posted. Changing the question may cause answer invalidation. Everyone needs to be able to see what the reviewer was referring to. What to do after the question has been answered. You can ask a new question \$\endgroup\$ Commented Nov 20, 2024 at 13:38

3 Answers 3

Reset to default
4
\$\begingroup\$

If your log object doesn't need to be assignable, consider using constant pointer to the pimpl object:

    std::unique_ptr<Impl> const pImpl;
    //                    🔺🔺🔺

It's not clear why the pimpl class uses this further level of indirection for its logger:

   std::shared_ptr<spdlog::logger> logger;

I think it would be better as an object member:

    spdlog::logger logger;

Or perhaps even inherit from it:

class logger::Impl : public spdlog::logger

In any case, we should initialise it in the constructor's init-list, rather than default-initialising and overwriting in the body:

    explicit Impl(spdlog::level::level_enum level)
       : logger{std::make_shared<spdlog::logger>("LOG")}
// or
    // : logger{"LOG"}   // as member object
// or
    // : spdlog::logger{"LOG"}   // as base class
    {

We have attempted perfect forwarding here:

    template <typename... T>
    void LOG(level_enum level, T... msg) {
        auto stream = dump(std::forward<T>(msg)...);
        print(level, stream);
    }

However, we got the function signature wrong - it needs to be T&&... msg.

And here, we forgot to forward at all:

   template <typename... T>
    std::ostringstream dump(T... msg) {
        std::ostringstream stream;
        ((stream << msg << ' '), ...);
        return stream;
    }

I think that should be

    template <typename... T>
    std::ostringstream dump(T&&... msg) {
        std::ostringstream stream;
        ((stream << std::forward<T>(msg) << ' '), ...);
        return stream;
    }
\$\endgroup\$
0
2
\$\begingroup\$

Thanks. I refined the code according to your first two feedbacks. Regarding perfect forwarding, I removed the dump method and also removed std::forward as it is not necessary. I just used a fold expression for the parameter pack, as below.

template <typename... T>
void LOG(level_enum level, T... msg) {
    std::ostringstream stream;
    ((stream << msg << ' '), ...);
    print(level, stream);
}
\$\endgroup\$
1
  • \$\begingroup\$ Self answers are acceptable, the answer contains one IO a what ("removed std::forward") and a why ("it is not necessary") so reaches the minimum bar for what is accepted as an answer. \$\endgroup\$ Commented Nov 13, 2024 at 8:25
2
\$\begingroup\$

Two things I need from a logging system:

My main issue is that your logger is "ALWAYS" generating work even when the logging statement is below the current log level. What I need from my logging system is that no functions are called if you are not going to log anything (I don't typically want to waste CPU in the core of my systems).

When I turn on logging for a difficult Hizenbug, I need trace-level debugging (and I should be able to enable logging on a running application (recompiling it is going to change it and, thus, potentially, its behavior).

Review

Your example:

    logger _logger(logger::trace);
    _logger.LOG(logger::debug, "test1", "test2", 1, 2, 3.1, 'X');

There is no work here, as all the values are compile-time constants. But that is not realistic. Generally, logging includes some information (especially debugging and tracing statements).

    logger _logger(logger::trace);
    _logger.LOG(logger::debug, "test1", "test2", getCount(), "Not Expected to exceed ", limit());

Here, you will evaluate the getCount() and limit() functions even if you do not log them.

More egregious, this always builds the actual logging string.

template <typename... T>
void LOG(level_enum level, T... msg) {
    auto stream = dump(std::forward<T>(msg)...);
    print(level, stream);
}

template <typename... T>
std::ostringstream dump(T... msg) {
    std::ostringstream stream;
    ((stream << msg << ' '), ...);
    return stream;
}

Unfortunately, the best way to do logging is by using macros and shortcut operators. You need to do the test for logging first, then evaluate any parameters. This is only really possible (and still looks neat) with macros.

#define Loging(Logger, level, ...)   (level >= Logger.CurrentLogLevel()) && Logger.dump( __VAR_ARGS__ )
\$\endgroup\$
1
  • \$\begingroup\$ In fact, my focus is on providing the user with a flexible interface so that they can implement any other third-party logging library. However, I totally agree with your point. Thanks for your feedback. I don't want to waste CPU too :) \$\endgroup\$ Commented Nov 20, 2024 at 12:27

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.