dlr-gtlab / gt-logging

Basic C++14 logging library based on QsLog
1 stars 1 forks source link

Setting logging level to OffLevel does not actually turn off the logging #123

Closed mariusalexander closed 9 months ago

mariusalexander commented 11 months ago

Setting gt::log::Logger::setLoggingLevel to OffLevel does not turn off the logging, instead all messages (even Trace) are logged.

Looking at the logging levels:

enum Level : int
{
    TraceLevel   =  1,
    DebugLevel   =  2,
    InfoLevel    =  4,
    WarningLevel =  8,
    ErrorLevel   = 16,
    FatalLevel   = 32,
    OffLevel     =  0 // <--- this one is less than all levels
};

The if statement of the logging macro (notice the operator <=):

// log only if logging level matches
#define GT_LOG_IMPL_IF_LEVEL(LEVEL) \
    if (gt::log::Logger::instance().loggingLevel() <= gt::log::LEVEL)

Now, either we change the logging level to have a larger value (> 32) or we fix the if statement.

Personally, I think writing <= is more convenient than having to check additionally whether the logging level is the OffLevel, thus I would only change the logging value (I propose 128).

rainman110 commented 11 months ago

I am wondering, if changing the value of OffLevel will introduce issues for tools that upgrade to the fixed logging.dll. Since they still use the old value (which is defined by the header), upgrading the DLL would not be sufficient.

On the other hand, the define GT_LOG_IMPL_IF_LEVEL is also in the header file, so even a fix here would no go into the DLL.

Hence, any solution require a rebuild for the depending libraries.

What about GTlab-core and the table model? Could it happen, that logging statements are now put into a wrong category since the enum has changed? Probably not, since it only affects the OffLevel, which is not a logging level.

mariusalexander commented 11 months ago

I have thought about this too. To make use of the new OffLevel value, plugins have to rebuild. Since the OffLevel would mostly be used by the application if at all (IMO) and is in general not used that often, I think "this is fine"