Merge lp://staging/~dobey/unity-api/add-simple-logger into lp://staging/unity-api

FAILED: Continuous integration, rev:278
https://unity8-jenkins.ubuntu.com/job/lp-unity-api-ci/167/
Executed test runs:
    FAILURE: https://unity8-jenkins.ubuntu.com/job/build/4785/console
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-0-fetch/4813
    FAILURE: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/4636/console
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/4636
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/4636/artifact/output/*zip*/output.zip
    FAILURE: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/4636/console
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/4636
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/4636/artifact/output/*zip*/output.zip
    FAILURE: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/4636/console
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/4636
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/4636/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-unity-api-ci/167/rebuild

PASSED: Continuous integration, rev:279
https://unity8-jenkins.ubuntu.com/job/lp-unity-api-ci/168/
Executed test runs:
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build/4787
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-0-fetch/4815
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/4638
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/4638/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/4638
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/4638/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/4638
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/4638/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/4638
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/4638/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/4638
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/4638/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/4638
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/4638/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-unity-api-ci/168/rebuild

This doesn't strike me as the correct approach. All the overloads of operator<<() shouldn't be necessary. Why not use an approach more like the Logger class in scopes-api?

It uses a LogStream class that derives from std::ostringstream and stores a reference to the actual ostream to write to. That makes it unnecessary to overload operator<<(), and all the usual mechanisms for allowing user-defined types to be inserted can still be used (such as adding an overloaded operator<<() for a custom type to namespace std).

LogStream can only be move constructed, so there are no life cycle issues with the reference to the real ostream.

I would do away with the whole maybe_space() thing altogether. I find it a *huge* pain in the butt that qDebug() and its ilk insist on adding spaces all the time. More often than not, when cobbling messages together, I end up having to use nospace() (and noquote()) all the time to stop it from messing with my output.

I'd *much* prefer having something that behaves just like a normal ostream. With that, I can also use manipulators to change formatting, and use all the other good things that come with ostreams.

Other niceties, such as adding a time stamp, are possible that way too.

The macros for debug(), warn(), etc. truly suck. Please let's not do this. They tear right through all the namespaces and can cause truly strange errors due to name clashes with something else called "debug" elsewhere (which is quite likely). We can just return a LogStream by value from a function instead (and the implementation of the function can tie it to the correct output stream).

It's not clear to me that this should be a header-only implementation. I'm with you on the desire for low overhead. But this is a heavy-weight implementation already because each and every message that is written hits the heap (and ostreams are not light-weight either).

In the interest of ABI stability, I strongly suspect that it would be better to pimpl the entire implementation. In terms of performance, it will not make an iota of difference, but it'll shield us from problems down the track in terms of ABI stability and applications not picking up new features until they are recompiled (as is the case for header-only).

Well, I made the Logger class an ostringstream based class, got rid of all the operator overrides, moved the ctor/dtor bits into a compiled file, and made it much simpler.

However, the macros have to be the way they are, because of the dependency on the G_LOG_DOMAIN value being defined at compile time. I wasn't able to get around this yet, but am open to suggestions. The goal with this was to have something as simple to use as qDebug() but which uses the g_log functionality, so we automatically get the G_DEBUG_MESSAGES env var handling at runtime, as a means of being consistent across the platform.

I unfortunately am also not able to get the output buffer overriding working right in the tests. It seems to work fine for the debug() case, but fails for all the other cases, regardless of how I order them.

I looked at what's in unity-scopes-api and it seems a bit complex for what I was wanting to do here.

FAILED: Continuous integration, rev:280
https://unity8-jenkins.ubuntu.com/job/lp-unity-api-ci/170/
Executed test runs:
    FAILURE: https://unity8-jenkins.ubuntu.com/job/build/4851/console
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-0-fetch/4879
    FAILURE: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/4690/console
    FAILURE: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/4690/console
    FAILURE: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/4690/console
    FAILURE: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/4690/console
    FAILURE: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/4690/console
    FAILURE: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/4690/console

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-unity-api-ci/170/rebuild

I don't see the need for macros at all. The G_LOG_DOMAIN seems to be a red herring. The underlying g_log() function accepts the domain as a parameter. As far as I can see, G_LOG_DOMAIN is used only by the g_error() and similar macros so you don't have to provide the domain at every point where a log message is produced.

We can do the same thing by having the logger instance hang onto the domain and supply it when a message is written with g_log(). No need for a macro that way.

We can't have the implementation without using a pimpl. It needs to be split into a public and internal API, otherwise we can't change the implementation without breaking ABI. The only data member of the Logger (and the LogWriter, see below) should be a unique_ptr to the implementation in the internal namespace.

Here is how I would approach it:

class Logger
{
public:
    Logger(std::string const& domain);

    Logger(Logger const&);
    Logger(Logger&&);
    Logger& operator=(Logger const&);
    Logger& operator=(Logger&&);
    virtual ~Logger();

    // ...
};

That's a well-behaved class with the big five taken care of. It does *not* derive from ostringstream. It simply is a class that remembers its domain in a data member (of the internal Impl class).

Now add a method to read the domain (for completeness) and a method that writes a log message:

class Logger
{
public:
    // ...

    std::string domain() const;
    LogWriter operator()(LogLevel l);
};

With that, I can write:

Logger log("mydomain");
log(LogLevel::debug) << "hello";

To make things a little more convenient, we can add these:

class Logger
{
public:
    // ...

    LogWriter operator()(); // Synonym for debug()
    LogWriter debug();
    LogWriter info();
    LogWriter message();
    LogWriter warning();
    LogWriter critical();
    LogWriter error();
};

Now I can write:

Logger log("mydomain");
log() << "debug";
log.warning() << "warning";

The LogWriter class takes care of writing the actual log message (note the derivation from ostringstream). I've omitted the pimpl here for brevity. The only data member should be a unique_ptr<internal::LogWriterImpl>; the two data members here should be part of LogWriterImpl:

class LogWriter : public std::ostringstream
{
public:
    LogWriter(LogWriter&&);
    LogWriter& operator=(LogWriter&&) = delete;
    ~LogWriter();

private:
    LogWriter(std::string const& domain);

    // There should be a unique_ptr<internal::LogWriteImpl> here instead.
    // This is only for exposition:
    std::string domain_;
    std::ostringstream str_;

    friend class Logger;
};

The LogWriter can be constructed with a domain only by the Logger. The API client can only move-construct a LogWriter, which enforces that the only way to use it is via the methods on Logger that return a LogWriter.

The LogWriter move constructor looks like this:

LogWriter::LogWriter(LogWriter&& other)
    : ostringstream(std::move(other))
    , domain_(other.domain_)
{
}

The destructor writes the message. Here, I'm just writing to clog, but the method could write using g_log() instead just as easily, passing in the domain:

LogWriter::~LogWriter()
{
    string msg = str();
    if (!msg.empty(...