variable initialization inside macro's if statement

Question

I'm a member of a university team designing a cubesat (nanosatellite). Another guy on the same subsystem was tasked to implement a logging library that we can use with the error stream. The core changes happen in two files, Logger.hpp and Logger.cpp, respectively.

He #defines different "log levels", each level corresponding to the severity of an error:

#if defined LOGLEVEL_TRACE
#define LOGLEVEL Logger::trace
#elif defined LOGLEVEL_DEBUG
#define LOGLEVEL Logger::debug
#elif defined LOGLEVEL_INFO
[...]
#else
#define LOGLEVEL Logger::disabled
#endif

Levels are inside of an enum:

enum LogLevel {
        trace = 32, // Very detailed information, useful for tracking the individual steps of an operation
        debug = 64, // General debugging information
        info = 96, // Noteworthy or periodical events
[...]
};

Additionally, he introduces the concept of "global level". That is, only errors with a level as severe as the global level's one or higher will be logged. To set the "global level", you need to set one of the constants mentioned above, such as LOGLEVEL_TRACE. More on that below.

Last but not least, he creates a custom stream and uses some macro magic to make logging easy, just by using the << operator:

template <class T>
Logger::LogEntry& operator<<(Logger::LogEntry& entry, const T value) {
    etl::to_string(value, entry.message, entry.format, true);

    return entry;
}

This question is about the following piece of code; he introduces a fancy macro:

#define LOG(level)
    if (Logger::isLogged(level)) \
        if (Logger::LogEntry entry(level); true) \
            entry

isLogged is just a helper constexpred function that compares each level with the "global" one:

static constexpr bool isLogged(LogLevelType level) {
        return static_cast<LogLevelType>(LOGLEVEL) <= level;
    }

I have never seen using macros like this, and before I go on with my question, here's his explanation:

Implementation details
This macro uses a trick to pass an object where the << operator can be used, and which is logged when the statement
is complete.

It uses an if statement, initializing a variable within its condition. According to the C++98 standard (1998), Clause 3.3.2.4, 
"Names declared in the [..] condition of the if statement are local to the if [...]
statement (including the controlled statement) [...]". 

This results in the Logger::LogEntry::~LogEntry() to be called as soon as the statement is complete.
The bottom if statement serves this purpose, and is always evaluated to true to ensure execution.

Additionally, the top `if` checks the sufficiency of the log level. 
It should be optimized away at compile-time on invisible log entries, meaning that there is no performance overhead for unused calls to LOG.

This macro seems cool, but makes me somewhat uneasy and my knowledge isn't sufficient to be able to form a proper opinion. So here goes:

• Why would anyone choose to go with implementing a design as this?
• What are the pitfalls to look out for with this approach, if any?
• (bonus) If this approach isn't considered good practice, what could be done instead?

What surprised (and alerted) me the most is that while the idea behind this doesn't seem too complicated, I couldn't find a similar example anywhere on the internet. I've come to learn that constexpr is my friend and that

1. macros can be dangerous
2. the preprocessor shouldn't be trusted

This is why a design built around a macro scares me, but I don't know whether this concern is valid, or whether it stems from my lack of understanding.

Lastly, I feel that I didn't phrase (and/or title) the question nearly as good as one could. So feel free to modify it :)

Answer 1

One issue here is that the macro parameter is used twice. If some function is called or some other expression with side effects is used within the LOG() argument, that expression (which need not be a constant expression) could be evaluated twice. Maybe not a big deal, since there's little reason in this case to use anything other than a direct LogLevel enumerator in LOG().

One more dangerous pitfall: consider code like

if (!test_valid(obj))
    LOG(Logger::info) << "Unexpected invalid input: " << obj;
else
    result = compute(obj);

Expanding the macro turns this into

if (!test_valid(obj))
    if (Logger::isLogged(Logger::info))
        if (Logger::LogEntry entry(Logger::info); true)
            entry << "Unexpected invalid input: " << obj;
        else
            result = compute(obj);

The compute function can never be called, no matter what the global log level is!

If your team does like this syntax, here's a way to get safer behavior. The if (declaration; expression) syntax implies at least C++17, so I assume other C++17 features. First, we'll need the LogLevel enumerators to be objects with different types so that the LOG expressions using them can have different behaviors.

namespace Logger {

template <unsigned int Value>
class pseudo_unscoped_enum
{
public:
    constexpr operator unsigned int() const noexcept
    { return m_value; }
};

inline namespace LogLevel {
    inline constexpr pseudo_unscoped_enum<32> trace;
    inline constexpr pseudo_unscoped_enum<64> debug;
    inline constexpr pseudo_unscoped_enum<96> info;
}

}

Next, define a dummy logger object that supports operator<< but does nothing.

namespace Logger {

struct dummy_logger {};

template <typename T>
dummy_logger& operator<<(dummy_logger& dummy, T&&)
{ return dummy; }

}

LOGLEVEL can keep its same macro definition. Finally, a couple of overloaded function templates replace the LOG macro (possibly in the global namespace):

#include <type_traits>

template <unsigned int Level,
          std::enable_if_t<(Level >= LOGLEVEL), std::nullptr_t> = nullptr>
LogEntry LOG(pseudo_unscoped_enum<Level>) { return LogEntry(Level); }

template <unsigned int Level,
          std::enable_if_t<(Level < LOGLEVEL), std::nullptr_t> = nullptr>
dummy_logger LOG(pseudo_unscoped_enum<Level>) { return {}; }

Answer 2

According to the description of if statement in cppreference.com, if you use an init-statement inside the if condition, like this:

if constexpr(optional) ( init-statement(optional) condition ) 
    statement-true 
else 
    statement-false

Then this will be equivalent to:

{
    init_statement 
    if constexpr(optional) ( condition ) 
        statement-true 
    else
        statement-false 
}

So, this means that in your case, the entry variable will go out of scope as soon as the scope of the whole if statement if finished. At this point, the destructor of the entry object is called and you will log some information about the instructions of the current scope. In addition, for using if constexpr statements, you should update your macro like this:

#define LOG(level)
    if constexpr (Logger::isLogged(level)) \
      ...

Why would anyone choose to go with implementing a design as this?

So, using if constexpr statements allows your to check a condition at compile time and if the condition if false, do not compile the statement-true. If you are using logging statements a lot in the code and you do not want to make your binary bigger when there is no logging necessary, you can go on with this approach.

What are the pitfalls to look out for with this approach, if any?

I see no specific pitfalls with this design. It is just complex to understand. This is one of those cases that you cannot replace macros with something else, e.g. template functions.