2

I have a logging class, which logs messages of any type:

/// log level enum
enum level_t
{
    DEBUG,   // used for debugging output
    CPLEX,   // used for cplex output
    INFO,    // used for generell informative output
    WARNING, // used for warning
    ERROR,   // used for error
};

/// converts a level_t type to a string
std::string lvl2str(level_t level)
{
    switch (level)
    {
    case DEBUG:
        return "debug";
    case CPLEX:
        return "cplex";
    case INFO:
        return "info";
    case WARNING:
        return "warning";
    case ERROR:
        return "error";
    default:
        throw std::runtime_error("there is no such log level");
    }
}

/// logger class.
///
/// use the log method to log information.
///
/// if you want to only log some information or simply filter the log output,
/// you can provide filter function (trough the constructor) in which you return true if a message should be logged
/// and false if it should not be logged. see the Logger::filter_t type for more information.
class Logger
{
  public:
    /// alias for the filter type.
    /// this is a function, which should return true, if a message should be logged, and false otherwise.
    /// you can log for example all messages with a log-level < INFO and/or messages starting with "MLU".
    /// this filter is extremely flexible, as it hands the logic to the user.
    using filter_t = std::function<bool(level_t, const std::string &)>;

  protected:
    /// default filter, used in the constructor.
    /// logs all messages
    constexpr static auto default_filter = [](level_t, const std::string &) { return true; };

  private:
    /// filter for filtering messages before logging them
    filter_t filter;

  public:
    Logger(filter_t filter = default_filter) : filter(std::move(filter))
    {
    }

    /// getter for the filter object
    filter_t get_filter() const
    {
        return this->filter;
    }

    /// logs a message, but only if the message passes the filter
    /// \tparam T type of the logged message
    /// \param level log level of the message
    /// \param filter function, which filters undesired log messages
    /// \param msg message to be logged
    template <typename T> static void log(level_t level, filter_t filter, const T &msg)
    {
        // convert the msg of template type T to a string, so we can apply the filter on the message
        std::string m = std::format("{}", msg);

        if (filter(level, m))
        {
            // get current local time
            time_t t = std::time(nullptr);
            std::tm tm = *std::localtime(&t);
            // convert time to string
            std::ostringstream oss;
            oss << std::put_time(&tm, "%H:%M:%S");
            auto time_str = oss.str();

            // generate log message
            auto log_msg = std::format("[{}] [{}] {}", time_str, lvl2str(level), msg);

            // log the message
            std::cout << log_msg << std::endl;
        };
    }

    /// logs a message, but only if the message passes the filter.
    /// this method also supports format strings.
    /// \tparam Args argument types for the format string
    /// \param level log level of the message
    /// \param filter function, which filters undesired log messages
    /// \param fmt format string
    /// \param args arguments for the format string
    template <typename... Args> static void log(level_t level, filter_t filter, const std::string &fmt, Args &&...args)
    {
        // append all args to a **single** message string
        auto formatted_msg = std::vformat(fmt, std::make_format_args(args...)); // todo [1]

        // pass the message to the logging function
        Logger::log(level, filter, formatted_msg);
    }


    /// logs a message, but only if the message passes the filter.
    /// same as the static log method, uses the filter object passed with the compiler.
    /// \tparam T type of the logged message
    /// \param level log level of the message
    /// \param msg message to be logged
    template <typename T> void log(level_t level, const T &msg)
    {
        Logger::log(level, this->filter, msg);
    }

    /// logs a message, but only if the message passes the filter.
    /// this method also supports format strings.
    /// same as the static log method, uses the filter object passed with the compiler.
    /// \tparam Args argument types for the format string
    /// \param level log level of the message
    /// \param fmt format string
    /// \param args arguments for the format string
    template <typename... Args> void log(level_t level, const std::string &format_string, Args &&...args)
    {
        Logger::log(level, this->filter, format_string, std::forward<Args>(args)...);
    }
};

and the example code:

int main(int argc, char *argv[])
{
    // add filter for creation of the logger
    Logger::filter_t filter = [](level_t, const std::string&){ return true; };
    // create logger class with filter
    Logger logger(filter);

    logger.log(level_t::INFO, "hello world"); // <-- [1] works as expected
    logger.log(level_t::INFO, "hello [{}] {}", "world", 42); // <-- [2] does not work

    return 0;
}

The method call marked with [1] works as expected, but the method call marked with [2] does not work, instead it exits with the following exit code: Process finished with exit code 139 (interrupted by signal 11: SIGSEGV). I debugged the program and it seems, that in the method template <typename... Args> static void log(level_t level, filter_t filter, const std::string &fmt, Args &&...args) the call Logger::log(level, filter, formatted_msg); calls the same method/ itself! This is not the behavior, which I would expected. Instead I expect, the method to call the template <typename T> static void log(level_t level, filter_t filter, const T &msg) method, because I didn't provide a pack parameter.

  1. Why does the error appear even though I have a overloaded function (same name, but different parameters)?
  2. How can I fix the error?
Moritz
  • 378
  • 5
  • 14
  • 2
    [`std::vformat`](https://en.cppreference.com/w/cpp/utility/format/vformat) returns a `std::string`, which perfectly matches the parameter-pack variant, with an empty parameter-pack. – Some programmer dude Apr 27 '23 at 13:46
  • 1
    You could simply rename or add a private member function to resolve the ambiguity. Or you could use `std::enable_if` to check that `sizeof...(Args) > 0`. – François Andrieux Apr 27 '23 at 13:48
  • The usage of `Logger` in your main doesn't match the `Logger` class you provided. It looks like you forgot to pass a `filter` object to the `log` function calls. It is also unclear why there is a `filter` data member when the `log` member functions are `static`. – François Andrieux Apr 27 '23 at 13:53
  • 2
    Change the type of `fmt` to `std::format_string` and call `std::format`. Also `std::format` has direct support for `std::chrono`; try that instead of `time_t` and string manipulation. – Red.Wave Apr 27 '23 at 16:10
  • @FrançoisAndrieux you are right, the main example and the log call does not match. but i just forgot to include the non static methods, which just call the static methos. i added the non static methods, because they are mandatory and the static methods are just a shorthand method call. – Moritz May 02 '23 at 11:44
  • @Red.Wave > Change the type of fmt to std::format_string and call std::format< Sadly I get the error `error: ‘format_string’ is not a constant expression`. Do you know how to fix this? > Also std::format has direct support for std::chrono < I will definitely try that! thanks for the hint. – Moritz May 02 '23 at 11:47
  • 1
    @Red.Wave Okay, I tinkered a bit around and figured out, that when I use `std::format_string` as my format string parameter type but still call `vformat`, I get no `not a constant expression` error and the calls from the main example word as expected! Tank you for your help. When you add your comments as an answer, I will accept that. – Moritz May 02 '23 at 11:58
  • I should solve that error too. It takes a bit of time. – Red.Wave May 02 '23 at 15:27

2 Answers2

2

For instance, merely rename

template <typename T> static void log(level_t level, filter_t filter, const T &msg)

to

template <typename T> static void logmsg(level_t level, filter_t filter, const T &msg)

and change

// pass the message to the logging function
Logger::log(level, filter, formatted_msg);

into

// pass the message to the logging function
Logger::logmsg(level, filter, formatted_msg);

otherwise the version with variadic argument will match. here is a running instance: https://godbolt.org/z/KKr6E4o5s

Edit: to be clearer, I think that the second function is a better match because:

you can see what happen if you keep the same name for both functions but remove the first template:

static void log(level_t level, filter_t filter, const std::string &msg)

live: https://godbolt.org/z/7YvfnKMEn

Oersted
  • 769
  • 16
  • That is a good and valid idea, but I don't want to change the name, but deliberately use the overloading feature. I want my logging class to be as flexible as possible. Still many tanks for your answer. – Moritz May 02 '23 at 12:01
1

All in all, a total redesign of the code seems essential. The none-variadic overload of Logger::log seems to be preparing a time prefix. I would initially recommend std::chrono for time, but everything regarding time and locality has a tone of caveats, and standard portable solutions might fail at compile or runtime - depending on platform.

Regardless of the time formatting issues, the actual API function the OP seems to need is the none-static variadic template. The static version seems to have convenience usage too. But the none-variadic version can simply be removed from the API, in order to totally avoid the overload resolution process.

In order to enable compile-time diagnostics for logging argument mismatch - instead of runtime errors - C++20 provides std::format_string:

private:
static std::ostream& log_prefix(level_t level){
    return std::cout << FORMATED_TIME << FORMAT_LEVEL(level);
    //TODO: FORMATED_TIME and FORMAT_LEVEL should be handled
};

public:

template <typename... Args>
static void log(level_t level, filter_t& filter, const std::format_string<Args ...> fmt, Args &&...args){
     auto msg = std::format(fmt, args...);
     if (!filter(level, msg))
         return;
     Logger::log_prefix(level) << msg;
};

template <typename... Args>
void log(level_t level, const std::format_string<Args ...> fmt, Args &&...args){
    Logger::log(level, this->filter, fmt, std::forward<Args>(args)...); 
};

However, I would recommend the OP to choose a different approach on filtering the message; E.G. check the formatting onlyfilter(level, fmt.get()), or other light weight strategies. because formatting already allocates and calculates the printable result.

Red.Wave
  • 2,790
  • 11
  • 17
  • I removed the static members, but I HAVE TO keep the none-static methods. They are essential for the functionality of my logger (you cannot see it, but in other parts of my code I depend on it). Sadly I cannot use std::chrono, because I had to swap to the fmt library, because our reference system don't support g++13, and I get nasty compilation errors when using chrono in conjunction with fmt. so i better wait for a g++13 package to be released. using `std::format_string` indeed did the trick! I dont get your last part. How can I use these lightweight solutions? – Moritz May 03 '23 at 10:10
  • I am not sure about your design. There are a lot of details rhat I can't see. I don't know what you filter out. You are using a `std::function` and I guess that's not what you need. You might want to pass filtering info at compile time(have a template logger). But I don't know what you want, and this was not the Canon part of the question. – Red.Wave May 03 '23 at 17:36