4
\$\begingroup\$

This class has a Log function that appends text to a log text file. It takes a format string and a variable amount of arguments, much like a printf sort of function. It then writes the formatted string with the interpolated arguments to the file.

I wanted to know if there are any edge cases I might've missed, or if anything can be done to improve this code?

class Logger {
private:
    Logger() = delete;
    Logger(const Logger&) = delete;
public:
    template <class... FormatArgs>
    static void Log(const std::string_view format, FormatArgs&&... args) {
        std::ofstream logfile("log.txt", std::ios::app);
        logfile << std::vformat(format, std::make_format_args(args...)) << std::endl;
    }
};

Edit

Here are some usage examples*.
Logger::Log("This is an example log message");
Logger::Log("This is a log message with {}", "one argument");
Logger::Log("This is a log message with {} {}", 2, "arguments");

Should produce a log.txt text file in the process's working directory with the following contents.

This is an example log message
This is a log message with one argument
This is a log message with 2 arguments
\$\endgroup\$
0

2 Answers 2

7
\$\begingroup\$

The class seems pointless, since it can't even be instantiated. Why not a free function (in a suitable namespace, perhaps)?

The function itself opens and closes log.txt each time it's called. That's pretty inefficient, and will cause that file to be appended or created uncontrollably wherever the process's working directory happens to be. I'm not sure I'd want any program doing that to my files (and it will be many files if the process frequently changes directory).

It's impossible to test in isolation, as it can't be decoupled from the file-system. We'd want to be able to write to a std::ostringstream in our tests, and we might want to write to the syslog service or equivalent in a daemon program, to take advantage of OS services such as log forwarding.

The template with forwarding reference arguments (FormatArgs&&) fails to std::forward() them when they are used.

There's really no need to flush output using std::endl when we immediately close the file, since that also flushes. Just use plain \n instead. Flushing output would be sensible if we kept the file open, rather than opening for each call.

We probably want to catch std::format_error, and perhaps also std::ios_base::failure and std::bad_alloc, as it's better for logging to fail than to bring down the whole program (unless it's an audit log, I guess). Perhaps the most flexible option is to return a status value that the caller can choose to ignore or to act on.

Perhaps we should be accepting a std::format_string as first argument (which would enable compile-time checking of its validity) rather than relying on run-time exceptions?


Modified code

#include <filesystem>
#include <format>
#include <fstream>
#include <string_view>

class Logger
{
    std::ofstream file = {};
    std::ostream& stream;

public:
    explicit Logger(std::ostream& os)
        : stream{os}
    {}
    explicit Logger(std::filesystem::path& filename)
        : file{filename, std::ios::app},
          stream{file}
    {}

    // This one has compile-time checking
    template<typename... Args>
    bool log(std::format_string<Args...> format, Args&&... args)
    {
        try {
            stream << std::format(format, std::forward<Args>(args)...)
                   << std::endl;
            return true;
        } catch (...) {
            return false;
        }
    }

    // Run-time checking only
    template<typename... Args>
    bool log_unchecked(std::string_view format, Args&&... args)
    {
        try {
            stream << std::vformat(format, std::make_format_args(args...))
                   << std::endl;
            return true;
        } catch (...) {
            return false;
        }
    }
};


#include <gtest/gtest.h>
#include <sstream>

TEST(Logger, Log)
{
    auto s = std::ostringstream{};
    auto logger = Logger{s};

    EXPECT_TRUE(logger.log("{} {}!", "Hello", "world", "junk"));
    EXPECT_EQ(s.str(), std::string{"Hello world!\n"});

    // logger.log("{} {}!", "Hello"); // doesn't compile
    EXPECT_FALSE(logger.log_unchecked("{} {}!", "Hello"));
}

For an alternative form of error checking, we could pass a flag to choose whether failures during logging should throw or not. This allows us to preserve the exception's type and content, which may be useful. We can use an extra argument to select the run-time-checked version instead of compile-time-checked (or mix and match parts of each change). That looks like:

    enum log_tag{ unchecked };
    enum log_flags{ throw_on_error = 1 };


    // This one has compile-time checking
    template<typename... Args>
    void log(log_flags flags, std::format_string<Args...> format, Args&&... args)
    {
        try {
            stream << std::format(format, std::forward<Args>(args)...)
                   << std::endl;
        } catch (...) {
            if (flags & throw_on_error) {
                throw;
            }
        }
    }
    template<typename... Args>
    void log(std::format_string<Args...> format, Args&&... args)
    {
        log(log_flags{}, format, std::forward<Args>(args)...);
    }

    // Run-time checking only
    template<typename... Args>
    void log(log_tag, log_flags flags, std::string_view format, Args&&... args)
    {
        try {
            stream << std::vformat(format, std::make_format_args(args...))
                   << std::endl;
        } catch (...) {
            if (flags & throw_on_error) {
                throw;
            }
        }
    }
    template<typename... Args>
    void log(log_tag tag, std::string_view format, Args&&... args)
    {
        log(tag, log_flags{}, format, std::forward<Args>(args)...);
    }

Corresponding changes to tests:

    auto s = std::ostringstream{};
    auto logger = Logger{s};

    logger.log("{} {}!", "Hello", "world", "junk");
    EXPECT_EQ(s.str(), std::string{"Hello world!\n"});

    // logger.log("{} {}!", "Hello"); // doesn't compile
    // std::string fmt{"{}!"}; logger.log(fmt, "Hello"); // doesn't compile

    s.str({});
    logger.log(Logger::unchecked, "{} {}!", "Hello");
    EXPECT_EQ(s.str(), "");

    EXPECT_THROW(logger.log(Logger::unchecked, Logger::throw_on_error, "{} {}!", "Hello"),
                 std::exception);
\$\endgroup\$
7
  • \$\begingroup\$ Does accepting an std::format_string as an argument negate the need to catch an std::format_error since the string is being validated at compile-time? Or is it always better to be safer than sorry? \$\endgroup\$
    – vince.smdt
    Dec 11, 2022 at 15:25
  • 1
    \$\begingroup\$ Yes, that's what the specification says: std::vformat() can throw a format error, but std::format() cannot. And the std::format_string constructor does all its checking at compile-time, so doesn't throw. \$\endgroup\$ Dec 11, 2022 at 16:04
  • \$\begingroup\$ Arguments to make_format_args shouldn't be forwarded, see e.g. eel.is/c++draft/format#functions. \$\endgroup\$
    – vitaut
    May 29, 2023 at 0:19
  • \$\begingroup\$ @vitaut, I don't see where the specification implies they shouldn't be forwarded - can you elaborate further? Thanks. \$\endgroup\$ Jun 1, 2023 at 7:00
  • 1
    \$\begingroup\$ I see. I'd like to understand why - I guess it's because the format string object keeps references to its constructor args rather than copying them, so forwarding is unnecessary (and obviously less readable). I'm accepting your edit suggestion - thanks for that, @vitaut. \$\endgroup\$ Jun 1, 2023 at 14:38
3
\$\begingroup\$

Just use std::format()

There is no point in using std::vformat() here, you can use std::format() directly:

logfile << std::format(format, std::forward<Args>(args...)) << std::endl;

Using std::vformat() would only be useful if Log() itself took a parameter of type std::format_args. Now you are just making the code less efficient than it has to be.

The only problem is that for std::format() to work, format must be a valid std::format_string, not a std::string_view. But that's probably desirable anyway (see Toby's answer).

template <class... Args>
static void Log(const std::format_string<Args...> format, Args&&... args) {
    …
}

Don't open the log file for every log entry

I would expect a class Logger to open the log file once and store the std::ofstream as a member variable, so it doesn't have to reopen the file every time Log() is called.

Missing error handling

The code is missing any form of error handling. What if the log file cannot be opened or a new entry cannot be appended?

\$\endgroup\$
3
  • 1
    \$\begingroup\$ Can you really use std::format() with a string-view instead of a std::format_string? The format-string's constructor is consteval, and it can't do the required checking for a non-constexpr string. \$\endgroup\$ Dec 7, 2022 at 11:59
  • 1
    \$\begingroup\$ Oof, you are right. You can pass it a non-constexpr std::string_view though, but that only works if Log() is made consteval... \$\endgroup\$
    – G. Sliepen
    Dec 7, 2022 at 22:13
  • \$\begingroup\$ consteval void Log() would be strange indeed! \$\endgroup\$ Dec 8, 2022 at 7:56

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.