Logger class (C++)

Question

I wrote a simple logger class for my project. Some key points:

• The interface is heavily inspired by Java's logging libraries (you can tell from the getters/setters that I'm a Java programmer). In particular I'm using printf style functions instead of std::ostream style, which I don't like very much.
• verify is a macro similar to assert, but instead of aborting it throws std::logic_error and cannot be disabled with NDEBUG.
• The heavy lifting is done by Handler, which I didn't include here because it's quite a lot of code and it's not finished.

Without further ado, here's log.hpp

#pragma once

#include "verify.hpp"

#include <cassert>
#include <ostream>
#include <utility>

namespace bran {
    enum class log_severity : unsigned {
        INHERIT,
        TRACE,
        DEBUG,
        INFO,
        WARN,
        ERROR,
        FATAL
    };

    std::ostream& operator<<(std::ostream& out, log_severity severity);

    template<class Handler>
    class basic_logger;

    template<class Handler>
    void swap(basic_logger<Handler>& a, basic_logger<Handler>& b) noexcept;

    template<class Handler>
    class basic_logger {
    public:
        explicit basic_logger(const std::string& name, basic_logger<Handler>* parent, Handler* handler);
        explicit basic_logger(const std::string& name, basic_logger<Handler>& parent);
        explicit basic_logger(const std::string& name, Handler& handler);

        basic_logger(basic_logger&& l) noexcept;
        basic_logger(const basic_logger&) = delete;

        ~basic_logger() = default;

        basic_logger& operator=(basic_logger l) noexcept;

        [[nodiscard]] inline std::string get_name() const noexcept;

        void set_threshold(log_severity threshold);
        [[nodiscard]] inline log_severity get_threshold() const noexcept;

        void set_handler(Handler* handler);
        [[nodiscard]] inline Handler* get_handler() const noexcept;

        void set_parent(basic_logger<Handler>* parent);
        [[nodiscard]] inline basic_logger<Handler>* get_parent() const noexcept;

        [[nodiscard]] inline bool is_trace() const noexcept;
        [[nodiscard]] inline bool is_debug() const noexcept;
        [[nodiscard]] inline bool is_info() const noexcept;
        [[nodiscard]] inline bool is_warn() const noexcept;
        [[nodiscard]] inline bool is_error() const noexcept;

        template<class... T>
        inline void trace(const std::string& message, T... args);
        template<class... T>
        inline void debug(const std::string& message, T... args);
        template<class... T>
        inline void info(const std::string& message, T... args);
        template<class... T>
        inline void warn(const std::string& message, T... args);
        template<class... T>
        inline void error(const std::string& message, T... args);
        template<class... T>
        inline void fatal(const std::string& message, T... args);

    private:
        std::string name;
        log_severity threshold;
        basic_logger<Handler>* parent;
        Handler* handler;

        template<class... T>
        inline void log(log_severity severity, const std::string& message, T... args);

    public:
        friend void swap<Handler>(basic_logger<Handler>& a, basic_logger<Handler>& b) noexcept;
    };
}

template<class Handler>
bran::basic_logger<Handler>::basic_logger(const std::string& name, basic_logger<Handler>* parent, Handler* handler)
        : name{name}, parent{parent}, handler{handler} {
    verify(!name.empty() && (parent != nullptr || handler != nullptr));
    if (parent != nullptr) {
        threshold = log_severity::INHERIT;
    } else {
        threshold = log_severity::TRACE;
    }
}

template<class Handler>
bran::basic_logger<Handler>::basic_logger(const std::string& name, basic_logger<Handler>& parent)
        : basic_logger{name, &parent, nullptr} {
}

template<class Handler>
bran::basic_logger<Handler>::basic_logger(const std::string& name, Handler& handler)
        : basic_logger{name, nullptr, &handler} {
}

template<class Handler>
bran::basic_logger<Handler>::basic_logger(basic_logger&& l) noexcept {
    swap(*this, l);
}

template<class Handler>
bran::basic_logger<Handler>& bran::basic_logger<Handler>::operator=(basic_logger l) noexcept {
    swap(*this, l);
    return *this;
}

template<class Handler>
std::string bran::basic_logger<Handler>::get_name() const noexcept {
    return name;
}

template<class Handler>
void bran::basic_logger<Handler>::set_threshold(log_severity threshold) {
    verify(threshold != log_severity::INHERIT || parent != nullptr);
    this->threshold = threshold;
}

template<class Handler>
bran::log_severity bran::basic_logger<Handler>::get_threshold() const noexcept {
    assert(threshold != log_severity::INHERIT || parent != nullptr);
    return threshold == log_severity::INHERIT ? parent->get_threshold() : threshold;
}

template<class Handler>
void bran::basic_logger<Handler>::set_handler(Handler* handler) {
    verify(handler != nullptr || parent != nullptr);
    this->handler = handler;
}

template<class Handler>
Handler* bran::basic_logger<Handler>::get_handler() const noexcept {
    assert(handler != nullptr || parent != nullptr);
    return handler == nullptr ? parent->get_handler() : handler;
}

template<class Handler>
void bran::basic_logger<Handler>::set_parent(basic_logger<Handler>* parent) {
    verify(parent != nullptr || (handler != nullptr && threshold != log_severity::INHERIT));
    this->parent = parent;
}

template<class Handler>
bran::basic_logger<Handler>* bran::basic_logger<Handler>::get_parent() const noexcept {
    return parent;
}

template<class Handler>
bool bran::basic_logger<Handler>::is_trace() const noexcept {
    return get_threshold() <= log_severity::TRACE;
}

template<class Handler>
bool bran::basic_logger<Handler>::is_debug() const noexcept {
    return get_threshold() <= log_severity::DEBUG;
}

template<class Handler>
bool bran::basic_logger<Handler>::is_info() const noexcept {
    return get_threshold() <= log_severity::INFO;
}

template<class Handler>
bool bran::basic_logger<Handler>::is_warn() const noexcept {
    return get_threshold() <= log_severity::WARN;
}

template<class Handler>
bool bran::basic_logger<Handler>::is_error() const noexcept {
    return get_threshold() <= log_severity::ERROR;
}

template<class Handler>
template<class... T>
void bran::basic_logger<Handler>::trace(const std::string& message, T... args) {
    log(log_severity::TRACE, message, args...);
}

template<class Handler>
template<class... T>
void bran::basic_logger<Handler>::debug(const std::string& message, T... args) {
    log(log_severity::DEBUG, message, args...);
}

template<class Handler>
template<class... T>
void bran::basic_logger<Handler>::info(const std::string& message, T... args) {
    log(log_severity::INFO, message, args...);
}

template<class Handler>
template<class... T>
void bran::basic_logger<Handler>::warn(const std::string& message, T... args) {
    log(log_severity::WARN, message, args...);
}

template<class Handler>
template<class... T>
void bran::basic_logger<Handler>::error(const std::string& message, T... args) {
    log(log_severity::ERROR, message, args...);
}

template<class Handler>
template<class... T>
void bran::basic_logger<Handler>::fatal(const std::string& message, T... args) {
    log(log_severity::FATAL, message, args...);
}

template<class Handler>
template<class... T>
void bran::basic_logger<Handler>::log(log_severity severity, const std::string& message, T... args) {
    if (get_threshold() > severity) {
        return;
    }

    get_handler()->log(*this, severity, message, args...);
}

template<class Handler>
void bran::swap(basic_logger<Handler>& a, basic_logger<Handler>& b) noexcept {
    using std::swap;

    swap(a.name, b.name);
    swap(a.threshold, b.threshold);
    swap(a.parent, b.parent);
    swap(a.handler, b.handler);
}

And the puny log.cpp

#include "log.hpp"

std::ostream& bran::operator<<(std::ostream& out, bran::log_severity severity) {
    static const std::string SEVERITY_NAMES[] = {"INHERIT", "TRACE", "DEBUG", "INFO", "WARN", "ERROR", "FATAL"};
    static const size_t SEVERITY_COUNT = sizeof(SEVERITY_NAMES) / sizeof(SEVERITY_NAMES[0]);

    auto index = static_cast<size_t>(severity);
    assert(index < SEVERITY_COUNT);

    return out << SEVERITY_NAMES[index];
}

Answer 1

Here are some things that may help you improve your code.

Don't write getters and setters for every class

C++ isn't Java and writing getter and setter functions for every C++ class is not good style. Instead, move setter functionality into constructors and think very carefully about whether a getter is needed at all. In this code, there are some invariants being enforced, but I'd still be skeptical about the need for all of these setters and getters. For example, the whole chaining of raw pointers to loggers is rather suspect. While checks are made to verify the pointer isn't nullptr, a handler or parent could go out of scope and the class would still be using a non-nullptr but invalid pointer.

Minimize the interface

Is there really a compelling need to have an is_trace and related? I'd recommend simplifying the interface by omitting all of those. The user can just as easily write if(log.threshold <= log_severity::TRACE) {}.

Use better data structures

The code currently contains code for operator<< that looks like this:

std::ostream& bran::operator<<(std::ostream& out, bran::log_severity severity) {
        static const std::string SEVERITY_NAMES[] = {"INHERIT", "TRACE", "DEBUG", "INFO", "WARN", "ERROR", "FATAL"};
        static const size_t SEVERITY_COUNT = sizeof(SEVERITY_NAMES) / sizeof(SEVERITY_NAMES[0]);

    auto index = static_cast<size_t>(severity);
    assert(index < SEVERITY_COUNT);

    return out << SEVERITY_NAMES[index];
}

Since you're using the C++17 [[nodiscard]], you could also use std::string_view and std::array to make this cleaner and more modern.

#include <array>
#include <string_view>
std::ostream& bran::operator<<(std::ostream& out, bran::log_severity severity) {
    static constexpr std::array<std::string_view, 7> severity_names{"INHERIT", "TRACE", "DEBUG", "INFO", "WARN", "ERROR", "FATAL"};
    auto index = static_cast<size_t>(severity);
    assert(index < severity_names.size());
    return out << severity_names[index];
}

Also, it's better to avoid ALL_CAPS names. See ES.9

Rethink the existence of the class

As noted in the description of the code, all of the real work is being done by an unposted Handler function. Since the only useful things in the basic_logger class are the name and severity level, I'd suggest replacing the entire templated class with a concrete class that contains those two data items and whatever actual behavior is in Handler. The only required function in the Handler is apparently log. If you think you might need variations on the handler, simply make it a base class and make the log and destructor virtual.

Provide complete code to reviewers

This is not so much a change to the code as a change in how you present it to other people. Without the full context of the code and an example of how to use it, it takes more effort for other people to understand your code. This affects not only code reviews, but also maintenance of the code in the future, by you or by others. One good way to address that is by the use of comments. Another good technique is to include test code showing how your code is intended to be used.

Answer 2

I am not familiar with Java and I don't fully understand the purpose of Handler / parent - it would help us to better review the code if it was explained as pseudocode or via an interface.

• Considering the way get_handler() is implemented... do you really intend to change parent's handler at runtime? Can't you just set the handler at class instantiation or something? Also, currently, changing parent at runtime is not thread-safe.

• I doubt that you want your logger to be a template. If you use it across all your codebase then either all of the code is a template (which is very troublesome currently for developers and the compiler...) or you end up with using a single version of the logger. Instead consider usage of an interface class ILogHandler so that you could run applications with different handlers without them needing to know anything about it. Sure, calling a virtual function is slower than a regular function but it pales in comparison to everything else the logger needs to do upon a function call. Though, you'd need to implement in the logger what to do with T... args, not in the Handler.

• Add verbose_level, not just severity_level. At times you'd want to see info_level log from a high level function but ignore warning/errors logs from low level functions.
• Also consider writing implementation of short functions when declaring them. It doesn't hinder view of the whole class declaration and you can see the implementation without the need to search for it. I might be spoiled with Visual Studio but it allows to hide implementation of functions in its text editor so in this case even long functions provide little to hindrance when their implementation is written inside class declaration.
• Since you work with C++17, consider using std::string_view instead of const std::string&.

• Use std::forward<T>(args)... when you forward data in template functions as otherwise you might end up with unnecessary data copying:

  template<class Handler>
  template<class... T>
  void bran::basic_logger<Handler>::debug(const std::string& message, T... args) {
       log(log_severity::DEBUG, message, std::forward<T>(args)...);
  }