2
\$\begingroup\$

I am evaluating two implementations of a C++ logging mechanism.

The first implementation, below, unconditionally streams (calls the insertion operator) to the std::ostream& returned by a class's various warn()/info()/etc. functions. Those functions conditionalize which std::ostream& is returned: std::cout or a "do-nothing" std::ostream:

// log.h
#ifndef FOO_LOG_H
#define FOO_LOG_H

#include <ostream>

namespace foo {

class Log {
public:
  static std::ostream& warn();
  static std::ostream& info();
  static std::ostream& err();
  static std::ostream& debug();
};

#define CERR_ERRNO(x) \
  foo::Log::err() << x << "() error " << errno << "(" << strerror(errno) << ")" << std::endl

#define DEBUG(x) \
  foo::Log::debug() << "[D] | " << x << std::endl

#define ERR(x) \
  foo::Log::err() << "[E] | " << x << std::endl

#define INFO(x) \
  foo::Log::info() << "[I] | " << x << std::endl

#define WARN(x) \
  foo::Log::warn() << "[W] | " << x << std::endl

void set_log_level(const int& level);
}

#endif // FOO_LOG_H
// log.cpp
#include "log.h"
#include <iostream>
#include <streambuf>
#include <syslog.h>

namespace {

int log_level = LOG_DEBUG;

class NullBuffer : public std::streambuf {
public:
  int overflow(int c) { // Discard all output
    return traits_type::not_eof(c);
  }

  int sync() { // Do nothing
    return 0;
  }
} null_buffer;

std::ostream nout(&null_buffer);

}

namespace foo {

std::ostream& Log::warn() {
  return (log_level >= LOG_WARNING) ? std::cout : nout;
}

std::ostream& Log::info() {
  return (log_level >= LOG_INFO) ? std::cout : nout;
}

std::ostream& Log::err() {
  return (log_level >= LOG_ERR) ? std::cout : nout;
}

std::ostream& Log::debug() {
  return (log_level >= LOG_DEBUG) ? std::cout : nout;
}

void set_log_level(const int& level) {
  if (level >= LOG_EMERG && level <= LOG_DEBUG) {
    log_level = level;
  }
}

}

The second implementation, below, uses a different strategy: the macros are redefined as for-loops that define a Log instance within the loop scope; the loop-condition is initially conditional on the Log instance's log-level versus a separately-set threshold; if the condition is true (the for-loop is entered), content is streamed to to the Log instance's std::ostream while also setting a flag to ensure the for-loop is not entered a second time. I.e. the second implementation conditionalizes whether to perform any streaming at all.

// log.h
#ifndef FOO_LOG_H
#define FOO_LOG_H

#include <ostream>
#include <syslog.h>

namespace foo {

class Log {
private:
  int level_; ///< The log-level for this Log instance.

public:
  Log(int level);

  /** Whether this Log instance should log. */
  bool should_log();

  /** @return The std::ostream that this Log instance outputs to. */
  std::ostream& out();
};

#define CERR_ERRNO(x) \
  for (foo::Log log(LOG_ERR); log.should_log(); ) { \
    log.out() << x << "() error " << errno << "(" << strerror(errno) << ")" << std::endl; \
  }

#define DEBUG(x) \
  for (foo::Log log(LOG_DEBUG); log.should_log(); ) { \
    log.out() << "[D] | " << x << std::endl; \
  }

#define ERR(x) \
  for (foo::Log log(LOG_ERR); log.should_log(); ) { \
    log.out() << "[E] | " << x << std::endl; \
  }

#define INFO(x) \
  for (foo::Log log(LOG_INFO); log.should_log(); ) { \
    log.out() << "[I] | " << x << std::endl; \
  }

#define WARN(x) \
  for (foo::Log log(LOG_WARNING); log.should_log(); ) { \
    log.out() << "[W] | " << x << std::endl; \
  }

void set_log_level(const int& level);
}

#endif // FOO_LOG_H
// log.cpp
#include "log.h"
#include <iostream>
#include <streambuf>

namespace {

int log_level = LOG_INFO;

}

namespace foo {

Log::Log(int level)
: level_(level >= LOG_EMERG && level <= LOG_DEBUG ? level : LOG_DEBUG) {}

bool Log::should_log() {
  return level_ <= log_level;
}

std::ostream& Log::out() {
  level_ = log_level + 1;
  return std::cout;
}

void set_log_level(const int& level) {
  if (level >= LOG_EMERG && level <= LOG_DEBUG) {
    log_level = level;
  }
}

}

I think I can summarize the difference as follows: the first implementation conditionalizes whether to stream to std::cout or a "no-nothing" std::ostream instance, and the second implementation conditionalizes whether to stream, at all, to std::cout.
Both implementations are intended to be used by the DEBUG(x)/ERR(x)/etc. macros, so I think that usability/user-friendliness can be discounted as a criteria for judgement.

Question to the code-reviewing community: is either implementation "better"?
Criteria for "better" might be efficiency, code size, and memory consumption.
I'm open to other criteria I may have overlooked from consideration.

I have a gut feeling that conditionalizing whether or not to call an insertion operator at all is more efficient than unconditionally calling the insertion operator, even if the target std::ostream instance is effectively do-nothing -- but I'm not sure I have anything to back up that gut feeling.

Does any set of criteria make one of these two implementation objectively "better"?

\$\endgroup\$
0

1 Answer 1

1
\$\begingroup\$

Both implementations share an obvious flaw: the obvious place to write logs is to the standard log stream (std::clog), not the standard output stream.

namespace foo isn't a great choice - choose a name that's more informative.

The null-object pattern used by the first version is easier to use than the conditional version, which ends up only being usable via macros. It's much easier to eliminate the macros from the first version.

The second should be a little more efficient in the case where significant work is done to retrieve the values to be logged - e.g. if we have:

INFO(some_expensive_function(my_struct));

then the null-object version will perform the computation and stream it before throwing it away, whereas the conditional version won't evaluate the call at all.

As an enhancement, consider improving the macros so that they can be configured to prepend the file and line number of the call site - that can be very useful when you end up with very similar messages in different places.

\$\endgroup\$

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.