6
\$\begingroup\$

The goal of this code is to format any log to STDOUT and STDERR with the file and the line of where the call is made. I took inspiration from different C loggers that I found on the internet. I want it to be small and efficient while using C++17.

I'm a bit suspicious about the call to std::data at line 30 not being null-terminated.

#include <iostream>
#include <string_view>
#include <array>

enum Log_level
{
    L_SILENT = 0,
    L_ERROR = 1,
    L_INFO = 2,
    L_DEBUG = 3,
};

static Log_level log_level = L_ERROR;

static constexpr std::array log_colors 
{
    "",
    "\x1B[1;31m",
    "\x1B[1;34m",
    "\x1B[1;30m"
};

template<typename... Args>
void vlk_log_internal(const Log_level level, const std::string_view s, Args... args)
{
    std::FILE* output = (level == L_ERROR ? stderr : stdout);
    if (level <= log_level)
    {
        std::fprintf(output, "%s", log_colors[level]);
        std::fprintf(output, std::data(s), args...);
        std::fprintf(output, "\x1B[0m");
        std::fprintf(output, "\n");
    }
}

#define vlk_log(lvl, msg, ...) { vlk_log_internal(lvl, "[%s:%d] " msg, __FILE__, __LINE__, ##__VA_ARGS__); } (void)0

int main()
{
    log_level = L_SILENT;

    vlk_log(L_ERROR, "Error will not show");

    log_level = L_ERROR;

    vlk_log(L_ERROR, "Error %s", "msg");
    vlk_log(L_INFO, "Info will not show");

    log_level = L_INFO;

    vlk_log(L_INFO, "Info %s", "msg");
    vlk_log(L_DEBUG, "Debug will not show");

    log_level = L_DEBUG;

    vlk_log(L_DEBUG, "Debug %s", "msg");

    return 0;   
}

Compiler Explorer link

\$\endgroup\$
2
  • \$\begingroup\$ It is great that you have made progress with your code, but please leave the code in your question as is without incorporating feedback from answers; doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ Commented Jun 11, 2018 at 19:15
  • \$\begingroup\$ Updated version here \$\endgroup\$ Commented Jun 11, 2018 at 19:20

2 Answers 2

Reset to default
5
\$\begingroup\$

Looking at the code, it is clear that you took a part from existing C code. Overal the code is acceptable, although when I should accept it, I would recommend some changes.

As already indicated in another answer, you could use scoped enums. This would remove the L_ prefix, unfortunately if you would use <windows.h>, you have to find a new name for ERROR.

The log_colors variable uses some strings to color the terminal. A comment about the actual colors would be nice. Separate variables with clear names even better.

vlk_log_internal accepts variadic arguments, which is really good. You could use forwarding references to prevent expensive copies.

The output variable looks strange outside of the if-statement, you could move it in or introduce early return instead.

std::fprintf, is strange in C++ code, I mostly try to get rid of it. This because you have to know the ugly syntax of format strings and it is easy to make mistakes as passing an std::string instead of the char-ptr. This even leaks into your API.

Next up, you have a define which has to hide the function. You even nicely put something at the end to allow a semicolon. I don't like, however the other constants force you.

PS: I don't see std::data

\$\endgroup\$
6
  • \$\begingroup\$ Scoped enum would make the log line longer (by using the Log_level prefix every time). What could be a replacement for std::fprintf ? I feel like using C++'s std::cout would be more of a PITA than anything else. Plus in the future I can write a new macro to log to a file easily. I removed the call to std::data after a discussion with a friend (switched from std::string_view to c-string. I'll see what I can do about forwarding and edit my post. Thanks! \$\endgroup\$ Commented Jun 11, 2018 at 18:37
  • \$\begingroup\$ Why would you move away from string_view? \$\endgroup\$ Commented Jun 11, 2018 at 18:39
  • \$\begingroup\$ File streams are compatible with fstream \$\endgroup\$ Commented Jun 11, 2018 at 18:40
  • \$\begingroup\$ I moved away from string_view because of some null-terminated issues. You can look at the history of the post if you want to see the old code \$\endgroup\$ Commented Jun 11, 2018 at 18:42
  • \$\begingroup\$ That's indeed the problem if you use printf, you could look into boost.org/doc/libs/1_58_0/libs/format/doc/format.html (or write your own alternative) \$\endgroup\$ Commented Jun 11, 2018 at 18:45
1
\$\begingroup\$

Just a quick reminder about the existence of scoped enums and some examples of their usage.
Also when you have successive values in your enum it is enough to set the first member as it will continue counting from that. In your case however you don't even need that as they start with 0 by default.

\$\endgroup\$
1
  • \$\begingroup\$ But this will break all the access to log_colors, and add the Log_level:: prefix to all the calls to the enum \$\endgroup\$ Commented Jun 11, 2018 at 16:19

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.