7
\$\begingroup\$

This is a pet project done as an exercise in C++.

The following are currently out of the scope of my project but I am considering them. If you deem it important don't hesitate to comment on it.

  • Multiple output streams corresponding to log levels for safety and better control over output.
  • Thread safety. Logging is currenty not thread safe.
  • Tee stream that allows logging the same logs to multiple streams at once. Should provide additional safety and useful functionality.
  • Signal handle flush. Flushing in the case of program crash.

I have just started using doxygen so I'm leaving the comments in.

Logger.h

#include <iostream>
#include <string>
#include <functional>

/// logLevels define logging priority - trace being lowest and off being highest - off enables us to turn off logging easily to save on performance.
/// a log cannot be made with a logLevel off - see LOG(level). 
enum class logLevel 
{
    trace = 0,
    debug,
    info,
    warn,
    error,
    off
};

/// \brief Handles stream input and linebreaking.
///
/// Intended use is as a temporary object. This is important because line breaking is done on object destruction, to be precise - parameter endlineStrategy is executed in the destructor. 
/// Input to the ostream is done using the operator <<. The operator forwards the input to the std::ostream, so all standard overloads are available.
class StreamDelegate
{
public:
    StreamDelegate(std::ostream& os, std::function<void(std::ostream&)> endlineStrategy /**<executed on object destruction, it should handle line breaking*/);

    ~StreamDelegate();

    StreamDelegate(const StreamDelegate&) = delete;
    StreamDelegate& operator=(const StreamDelegate&) = delete;

    StreamDelegate(StreamDelegate&&) = default;
    StreamDelegate& operator=(StreamDelegate&&) = default;

    template<class T>
    inline StreamDelegate& operator<<(T&& output)
    {
        ostream << std::forward<T>(output);
        return *this;
    }

private:
    std::function<void(std::ostream&)> endline;
    std::ostream& ostream;
};

class Logger
{
public:
    ///\brief instantiates a global Logger object
    ///
    ///For now it is not possible to change globalLogLevel and output stream dynamically. 
    /// \todo dynamic setting of Loggers members globalLogLevel and logStream
    static Logger& instance(logLevel globalLogLevel = logLevel::off, std::ostream& output = std::cout);

    ~Logger();

    Logger(Logger const&) = delete;
    void operator=(Logger const&) = delete;
    ///\brief this is it
    StreamDelegate log(logLevel level);

    logLevel getGlobalLogLevel();
protected:
    void timeStamp();
    void logLevelStamp(logLevel level);
    std::function<void(std::ostream&)> endlineStrategy(logLevel level);

    std::string logLevelString(logLevel level);
private:
    Logger();
    Logger(logLevel globalLogLevel, std::ostream& output);

    std::ostream& logStream;
    const logLevel globalLogLevel;
};


/// \brief Use to initialize the Logger.
///
/// Once set globalLogLevel and output can't be changed.
#define LOGGER_INIT(globalLogLevel, output) Logger::instance(globalLogLevel, output); 

/// \brief Call for a Log of a certain level. Should be used only after initialization.
///   
/// if the check passes a call logging is enabled and input is done with the operator <<.
/// If the check fails code Logger::instance.log() isn't called and the following operator << calls arent made.
#define LOG(level) if(Logger::instance().getGlobalLogLevel() <= (level) && (level) < logLevel::off) Logger::instance().log(level)

I have read it might be wise to avoid macros. Macro LOG(level) shows a performance benefit over other solutions known to me. To avoid the use of Macros on log priority check fail a dummy stream would have to be created, and sent as an argument to StreamDelegate, so that following operator << calls aren't bad syntax.

Logger.cpp

#include "Logger.h"
#include <iomanip>



//PUBLIC LOGGER

Logger & Logger::instance(logLevel globalLogLevel, std::ostream& output) 
{
    static Logger instance(globalLogLevel, output);
    return instance;
}

Logger::~Logger()
{
}

StreamDelegate Logger::log(logLevel level)
{
    timeStamp();
    logLevelStamp(level);

    return StreamDelegate(this->logStream, this->endlineStrategy(level));
}

logLevel Logger::getGlobalLogLevel()
{
    return this->globalLogLevel;
}

//PROTECTED LOGGER
void Logger::logLevelStamp(logLevel level)
{
    // magic number - should I avoid it and how
    this->logStream << std::setw(7) << logLevelString(level) + ": ";
}

void Logger::timeStamp()
{
    this->logStream << __TIME__ << " ";
}

/// \todo avoid switch statement repetition
std::string Logger::logLevelString(logLevel level)
{
    //strategy?
    switch (level)
    {
        case logLevel::trace: return "TRACE";
        case logLevel::debug: return "DEBUG";
        case logLevel::info:  return "INFO";
        case logLevel::warn: return "WARN";
        case logLevel::error: return "ERROR";
        case logLevel::off: break;

        default: break;
    }
}
/// \todo avoid switch statement repetition
std::function<void(std::ostream&)> Logger::endlineStrategy(logLevel level)
{
    std::function<void(std::ostream&)> endlineNoFlush = [](std::ostream& os) {os << "\n"; };
    std::function<void(std::ostream&)> endlineFlush = [](std::ostream& os) {os << std::endl; };

    switch (level)
    {
    case logLevel::trace: return endlineNoFlush;
    case logLevel::debug: return endlineNoFlush;
    case logLevel::info:  return endlineNoFlush;
    case logLevel::warn: return endlineFlush;
    case logLevel::error: return endlineFlush;
    case logLevel::off: break;

    default: break;
    }
}

//PRIVATE LOGGER
Logger::Logger(logLevel globalLogLevel, std::ostream& output)
    : globalLogLevel(globalLogLevel), logStream(output)
{
}

//PUBLIC STREAM DELEGATE
StreamDelegate::StreamDelegate(std::ostream & os, std::function<void(std::ostream&)> endline)
    :
    ostream(os), endline(endline)
{
}

StreamDelegate::~StreamDelegate()
{
    endline(ostream);
}

I have the same switch case repeating in two methods - Logger::logLevelString and Logger::endlineStrategy. It seems small and I don't see much room for it to grow and cause readability issues. For the sake of good code, what would be a good way of avoiding this?

To demonstrate the use:

main.cpp

#include "Logger.h"

int main()
{
    LOGGER_INIT(logLevel::debug, std::cout);

    LOG(logLevel::trace) << "Dummy line. " << "No input.";
    LOG(logLevel::info) << "First line.";
    LOG(logLevel::warn) << "Second: first input. " << "Second line: second input.";

    std::cin.get();
}
\$\endgroup\$
2
\$\begingroup\$

You should always use as many compiler warnings as you can (for example: -Wall -Wextra -Weffc++ -pedantic). Some people encourage the use of -Werror which will turn all warnings into errors to enforce that people address their warnings.
In your case the warnings are easy to fix as it's only about initialization order and reaching the end of non-void functions.

The usage of __TIME__ might not be enough in the future. Maybe you should switch to an approach based on something like std::time.

Enums will start at 0 so you don't need to specifically set it.

Your lines are a bit long. The conservative number to break at is 80 characters, more liberal approaches say it's okay to go up to 120 but certainly not 187.

The straight-forward way to avoid magic numbers is to reform them into a constant. E.g. constexpr int some_arbitrary_width = 7; and then use that instead of the number.

Macros are not typesafe as they are purely text replacment done by the pre-processor. You say there is a performance difference, have you measured this?

Performance wise the switch statements are probably hard to beat and not the worst solution. Just make sure you fix the compiler warnings connected to those functions. That is to say always return a value.

For example:

switch (level)
{
    case logLevel::trace: return "TRACE";
    case logLevel::debug: return "DEBUG";
    case logLevel::info:  return "INFO";
    case logLevel::warn: return "WARN";
    case logLevel::error: return "ERROR";
    // fall through
    case logLevel::off:
    default: return "";
}

and the other one:

switch (level)
{
    case logLevel::trace: return endlineNoFlush;
    case logLevel::debug: return endlineNoFlush;
    case logLevel::info:  return endlineNoFlush;
    case logLevel::warn: return endlineFlush;
    case logLevel::error: return endlineFlush;
    // fall through
    case logLevel::off:
    default: return endlineFlush;
}

By falling through one case you end up in the default case where you return a value. As an alternative you can also explicitly return a value from all cases.

\$\endgroup\$
  • \$\begingroup\$ Thanks, i'll study your answer as soon as I get the time. The neat thing about the macro is that it "eats" up the << calls if the condition fails. Otherwise i need to send a dummy obj that defines operator<< . I think I'll do just that. \$\endgroup\$ – Antonio Dropulić Mar 28 '18 at 20:46
  • \$\begingroup\$ Hi @yuri. I've made improvements as you suggested. I have a few follow up questions. Do comments fall in the same linebreaking category as code? Is assert a good way of coping with reaching the end of non-void functions, in general and in this specific example (default switch value that shouldn't be hit). Thanks in advance :) \$\endgroup\$ – Antonio Dropulić Apr 2 '18 at 10:45
  • 1
    \$\begingroup\$ @AntonioDropulić (1) I would say that all code, including comments should adhere to whatever limit you impose. The reason for having a limit is usually so you can have 2 different windows side by side. It would defeat the point if some lines don't adhere to the limit. (2) I don't think assert is a good choice in this case. A better solution is to make use of fall-through cases to always return a statement. I will edit my post to make it more clear. \$\endgroup\$ – yuri Apr 2 '18 at 10:56
  • 2
    \$\begingroup\$ I'd argue against default in those switch blocks, as it inhibits the useful compiler warning when you add a new member to the enum but forget to update the switches. You do then need a catch-all return after the switch, of course. \$\endgroup\$ – Toby Speight Apr 2 '18 at 12:01
  • 1
    \$\begingroup\$ Usage of __TIME__ will also severely mess with most build systems, and even IF build systems knew about it, having a file that's always "dirty" is just bad for compile times and CI stability. \$\endgroup\$ – Frank Apr 2 '18 at 13:50
2
\$\begingroup\$

The LOG() macro is dangerous

Macros containing if tend to be prone to problems, particularly when the user's else can attach to the macro's if like this:

#include "Logger.h"

int main()
{
    LOGGER_INIT(logLevel::info, std::cerr);

    if (std::cin)
        LOG(logLevel::debug) << "Input is okay";
    else
        LOG(logLevel::warn) << "Input problem";
}

Improve switch statements

Consider endlineStrategy():

std::function<void(std::ostream&)> Logger::endlineStrategy(logLevel level)
{
    std::function<void(std::ostream&)> endlineNoFlush = [](std::ostream& os) {os << "\n"; };
    std::function<void(std::ostream&)> endlineFlush = [](std::ostream& os) {os << std::endl; };

    switch (level)
    {
    case logLevel::trace: return endlineNoFlush;
    case logLevel::debug: return endlineNoFlush;
    case logLevel::info:  return endlineNoFlush;
    case logLevel::warn: return endlineFlush;
    case logLevel::error: return endlineFlush;
    case logLevel::off: break;

    default: break;
    }
}

In any unhandled case, we go off the end of the switch, and fail to return a value. My preference is not to have a default case in the switch, as that inhibits the useful compiler warning when you add a new member to the enum but forget to update the switches. We can make endlineNoFlush and endlineFlush static const - or eliminate them entirely, by grouping the cases together:

static std::function<void(std::ostream&)> endlineStrategy(logLevel level)
{
    switch (level)
    {
    case logLevel::trace:
    case logLevel::debug:
    case logLevel::info:
         return [](std::ostream& os) { os << '\n'; };
    case logLevel::warn:
    case logLevel::error:
         return [](std::ostream& os) { os << std::endl; };
    case logLevel::off:
         break;
    }
    // shouldn't happen
    return [](std::ostream& os) {};
}

(I've also made it a non-member - you might go halfway and make it a static member, but it really has no need to be part of the class).

\$\endgroup\$
  • \$\begingroup\$ Thanks. About endline strategy belonging outside of the class. The argument would be that logLevel and its properties - in this case endlineStrategy - are not Logger invariants and do not in any way preserve it? I don't know why but somehow it bothers me that the logger does not control the endline strategy for given log levels. It's just my limited hunch :) \$\endgroup\$ – Antonio Dropulić Apr 2 '18 at 20:27
  • \$\begingroup\$ Also, should the same be done for timeStamp and logLevelStamp. All functions provide log formating. I do delegate the execution of the endlineStrategy, but I fail to see how that matters. \$\endgroup\$ – Antonio Dropulić Apr 2 '18 at 22:02
  • 1
    \$\begingroup\$ Yes, I'd say that timeStamp, logLevelStamp and logLevelString all belong with endlineStrategy, assuming that you want to keep them for re-use (as they are only used once each, you might want to inline them). \$\endgroup\$ – Toby Speight Apr 3 '18 at 7:58

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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