Merge lp:~jpakkane/unity-scopes-api/simplelogging into lp:unity-scopes-api/devel

[Jussi Pakkanen]

A very simple logging class.

A very simple logging class. Mostly a building block for further functionality.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:258
http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-ci/224/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-trusty-amd64-ci/225/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-trusty-armhf-ci/224/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-trusty-i386-ci/224/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-scopes-api-devel-ci/224/rebuild

review: Needs Fixing (continuous-integration)

[Paweł Stołowski (stolowski)]

Why not dumping to std::cerr right away in LogGatherer& LogGatherer::operator<< ? That would save us some extra string copying. Is there a good reason for buffering strings up like that?

review: Needs Information

[lp:~jpakkane/unity-scopes-api/simplelogging]

259. By Jussi Pakkanen on 2014-03-05

Simple locking.

[Jussi Pakkanen (jpakkane)]

Because dumping immediately causes interleaving for multiple threads.

Updated the MR to provide simple locking.

If we want this to be fully multithreaded we either need to copy the items or create a complex locking mechanism. Since the data sizes are minuscule (less than 1k for >99% of the time) and usage infrequent, let's just copy.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:259
http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-ci/226/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-trusty-amd64-ci/227/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-trusty-armhf-ci/226/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-trusty-i386-ci/226/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-scopes-api-devel-ci/226/rebuild

review: Needs Fixing (continuous-integration)

[lp:~jpakkane/unity-scopes-api/simplelogging]

260. By Jussi Pakkanen on 2014-03-05

Merged trunk.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:260
http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-ci/227/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-trusty-amd64-ci/228
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-trusty-armhf-ci/227
        deb: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-trusty-armhf-ci/227/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-trusty-i386-ci/227

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-scopes-api-devel-ci/227/rebuild

review: Approve (continuous-integration)

[Paweł Stołowski (stolowski)]

199 +static std::mutex print_mutex;

This could be a static private member of LogGatherer, no? I realise it's not exposed anywhere right now, but making it a static member would encapsulate it even more

Could you please provide overloaded << for all basic data types?

review: Needs Fixing

[Michi Henning (michihenning)]

> Could you please provide overloaded << for all basic data types?

I'd be inclined to hold off with that for the moment. There is no point in adding an operator<< for floats if we don't have any place in the code where we want to log a float. All it means is that we end up having to write more code and tests, for no gain in functionality.

[Michi Henning (michihenning)]

LogStream should be provided as an abstract base with only pure virtual functions. That way, we don't couple the logging mechanism to the call sites and we can replace it with something else without recompiling everything that logs something, and the choice of which logger to use can be delayed until run time.

[Michi Henning (michihenning)]

196 +namespace internal
197 +{
198 +
199 +static std::mutex print_mutex;
200 +
201 +LogStream errlog;

This doesn't look right. If the mutex is static, it has internal linkage, meaning that each compilation unit will end up with its own separate copy of the mutex, so locking the mutex will not achieve what it's meant to.

If we want to have all source files share the same single mutex, it needs to be a static member variable of the class.

[Michi Henning (michihenning)]

> This doesn't look right. If the mutex is static, it has internal linkage,

Ah, sorry, forget it. I confused source file and header file.

[Paweł Stołowski (stolowski)]

> > Could you please provide overloaded << for all basic data types?
>
> I'd be inclined to hold off with that for the moment. There is no point in
> adding an operator<< for floats if we don't have any place in the code where
> we want to log a float. All it means is that we end up having to write more
> code and tests, for no gain in functionality.

I think it's worth to have one for bool (and making sure it behaves as if std::boolalpha was set).

[Michi Henning (michihenning)]

Bool is fine with me, but let's not get carried away. If we find in two months time that we really want to log a double with formatting, we can still add something then.

[Jussi Pakkanen (jpakkane)]

You can totally change the logging framework without recompiling everything. You just change the .cpp file and recompile that (not-yet-done pimpling notwithstanding). There is no need to change the header for that.

As far as runtime goes, there are two things we might want to do then:

1. configure it (e.g. set log levels or the like)
2. change it (e.g. from Boost to glib or what have you)

The first one can be done just by adding some (global?) functions. A virtual base provides no improvements. The latter is something we don't ever need to do. Why on earth would we change between the two at runtime? We'll just pick one and be done with it. Even if for some strange reason we would want to, we can implement 2 in the same way as 1: a configuration option of the class.

Abstract base classes only make sense if you have multiple objects of the same type existing at the same time and need to be used interchangeably. We don't. It's just a simple "write this stuff to the log". It does _not_ need anything fancier because those features just complicate things for no good reason. As an example, let's look at this:

errlog << "Some text";

For this to work, errlog needs to be either a macro that does something magical or a concrete (that is, non-abstract) object. It can _not_ be an abstract base class because such an object is non-constructible. This is also the only way it is used. Thus:

1. we absolutely need a concrete, instantiated object with defined semantics
2. its type does not change at runtime (it behaviour might, but that's just a question of configuration)

Under these conditions, what is the benefit an abstract base class would bring?

("Because maybe we might sometime in the future do something perhaps" is NOT a valid answer, it is handwaving.)

[Michi Henning (michihenning)]

We have RuntimeImpl in the code as a handle to anything that the library needs. That gives us the ability to have multiple runtime instances in a process (we do this in the tests, for example), and it allows us to control the life time of each run time.

With a global logger variable, we can't do this anymore, and separate instances of the runtime in the same process will share the same logger.

I think it would be better to have RuntimeImpl own and initialise the logger. I believe that it's possible to get at the RuntimeImpl/Runtime instance from most places in the code. There may be places in value types where we would like to log something and can't get at the Runtime handle. I haven't checked this in detail yet, but I expect there won't be too many places in the code where this is the case. In most of them, we can probably make the logger available if we need it.

At any rate, I'm uncomfortable with having a global variable.

[Jussi Pakkanen (jpakkane)]

Not having global variables is a valid goal. To do that we either need to:

1. pass a logging provider to all classes that need logging (tedious, let's not do that)
2. have errlog class determine where the messages should go and configure LogGather to write its stuff there

Option 2 would mean roughly this in pseudocode:

if something
  l = LogGatherer(with some options)
else
  l = LogGatherer(with some other options)
endif

I would imagine this is what Qt does and why its logging classes are used like this:

qDebug() << "something";

[lp:~jpakkane/unity-scopes-api/simplelogging]

261. By Jussi Pakkanen on 2014-03-06

Now with bool support.

[Thomas Voß (thomas-voss)]

As this is an implementation detail, why not use google log (see https://code.google.com/p/google-glog). It is in main, and provides all of the required functionality. At any rate, establishing an interface as in:

class Logger
{
public:
    enum class Severity
    {
        error,
        warning,
        info,
        ...
    };

    virtual std::ostream& log(Severity severity) = 0;
};

with a GLog-based implementation would be quite helpful in testing and, later on, in real reporting use-cases.

For the global variable/global function approach: Singleton is an anti-pattern from my POV, and while it might be less tedious to just have a global logger instance, it results in tight coupling and makes isolated component testing really really difficult.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:261
http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-ci/230/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-trusty-amd64-ci/231
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-trusty-armhf-ci/230
        deb: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-trusty-armhf-ci/230/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-trusty-i386-ci/230

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-scopes-api-devel-ci/230/rebuild

review: Approve (continuous-integration)

[Jussi Pakkanen (jpakkane)]

If you look at glog, it uses singletons (or macros, which are "global functions" to use your terminology) too. You use it like this:

LOG(INFO) << "Something";

Also, could you give an example of where using a global logging function would make debugging harder? Please be specific, preferably by using sample code.

While you are at it, could you also explain how non-global logging objects should work? As an example let's say we have class A which internally uses class B, which uses class C, which in turn uses class D. Now let's suppose class D needs to write a log message. The questions to answer are:

- what object (not a function as those are forbidden, right?) should it call to send this message
- how is this object passed on to it
- who is the master owner of this object

[Thomas Voß (thomas-voss)]

> If you look at glog, it uses singletons (or macros, which are "global
> functions" to use your terminology) too. You use it like this:
>
> LOG(INFO) << "Something";
>

Sure, that's why I proposed a logger interface that can be implemented with the help of GLog to avoid a singleton use.

> Also, could you give an example of where using a global logging function would
> make debugging harder? Please be specific, preferably by using sample code.
>

For testing (which I originally referred to): Somewhere in your implementation (in a .cpp) you have one or multiple lines like:

  logerr << "...";

How do you test (easily) that the correct log string is generated? Of course, we could fall back to link time seams here, but I don't think that's a good idea.

> While you are at it, could you also explain how non-global logging objects
> should work? As an example let's say we have class A which internally uses
> class B, which uses class C, which in turn uses class D. Now let's suppose
> class D needs to write a log message. The questions to answer are:
>
> - what object (not a function as those are forbidden, right?) should it call
> to send this message
> - how is this object passed on to it
> - who is the master owner of this object

Hmmm, I might miss something, but D should look like:

  class D
  {
    public:
        D(const Logger::Ptr& logger) : logger(logger)
        {
        }

        void do_something()
        {
            logger->log(error) << "Something went horribly wrong";
        }
    private:
        Logger::Ptr logger;
  };

And yes, the logger dependency would have to be handed down the dependency chain. Nothing wrong with that from my perspective as the logger (and later on a reporter) in fact is a real functional requirement of class D. You will not get rid of that by satisfying it implicitly with a singleton.

The ultimate owner of the logger is the runtime.

[Jussi Pakkanen (jpakkane)]

> How do you test (easily) that the correct log string is generated?

By forwarding the log output to a file and inspecting its contents.

> And yes, the logger dependency would have to be handed down the dependency chain.

Please read this:

http://steve-yegge.blogspot.fi/2006/03/execution-in-kingdom-of-nouns.html

> Nothing wrong with that from my perspective

But from the perspective of the poor bastards who have to augment, basically, EVERY SINGLE CLASS in the entire source base with extra constructor arguments this is a very big problem. These people have better things to do with their time.

[Thomas Voß (thomas-voss)]

> > How do you test (easily) that the correct log string is generated?
>
> By forwarding the log output to a file and inspecting its contents.
>
> > And yes, the logger dependency would have to be handed down the dependency
> chain.
>
> Please read this:
>
> http://steve-yegge.blogspot.fi/2006/03/execution-in-kingdom-of-nouns.html
>

I would hold http://www.mockobjects.com/2007/04/test-smell-logging-is-also-feature.html against that and your proposal for testing.

> > Nothing wrong with that from my perspective
>
> But from the perspective of the poor bastards who have to augment, basically,
> EVERY SINGLE CLASS in the entire source base with extra constructor arguments
> this is a very big problem. These people have better things to do with their
> time.

Two things:

(1.) why would every class need to log? Littering production code with logging message sounds like bad idea. Instead, instrumenting an executable with http://gcc.gnu.org/onlinedocs/gcc/Code-Gen-Options.html#index-finstrument_002dfunctions-2112 and implementing the respective hooks might be a better idea.

(2.) Under the assumption of non-universal logging (see (1.)), I would strongly assume that the scope of the changes is limited and well-defined in reality as only certain classes would need to be provided with logging capabilities.

[Michi Henning (michihenning)]

Cleaning up moldy oldies.

review: Disapprove

Unmerged revisions

261. By Jussi Pakkanen on 2014-03-06

Now with bool support.

260. By Jussi Pakkanen on 2014-03-05

Merged trunk.

259. By Jussi Pakkanen on 2014-03-05

Simple locking.

258. By Jussi Pakkanen on 2014-03-05

Start using the logging class instead of hitting std::cerr directly.

257. By Jussi Pakkanen on 2014-03-05

Now with numbers.

256. By Jussi Pakkanen on 2014-03-05

Some more test cases.

255. By Jussi Pakkanen on 2014-03-05

Very basic output plus some text.

254. By Jussi Pakkanen on 2014-03-05

Start work on logging classes.

=== added file 'include/unity/scopes/internal/Logging.h'
--- include/unity/scopes/internal/Logging.h	1970-01-01 00:00:00 +0000
+++ include/unity/scopes/internal/Logging.h	2014-03-06 09:05:05 +0000
@@ -0,0 +1,70 @@
+/*
+ * Copyright (C) 2014 Canonical Ltd
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License version 3 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Jussi Pakkanen <[e-mail]>
+ */
+
+#ifndef UNITY_SCOPES_INTERNAL_LOGGING_H
+#define UNITY_SCOPES_INTERNAL_LOGGING_H
+
+#include <string>
+#include <vector>
+#include <unity/util/NonCopyable.h>
+
+namespace unity
+{
+
+namespace scopes
+{
+
+namespace internal
+{
+
+class LogGatherer;
+
+class LogStream final {
+    NONCOPYABLE(LogStream);
+public:
+    LogStream();
+    LogGatherer operator<<(const std::string &s);
+    LogGatherer operator<<(const int i);
+    LogGatherer operator<<(const char *msg);
+    LogGatherer operator<<(const bool i);
+};
+
+class LogGatherer final {
+
+public:
+
+    LogGatherer();
+    ~LogGatherer();
+    LogGatherer& operator<<(const std::string &s);
+    LogGatherer& operator<<(const int i);
+    LogGatherer& operator<<(const bool i);
+    LogGatherer& operator<<(const char *msg);
+
+private:
+    std::vector<std::string> strings;
+};
+
+extern LogStream errlog;
+
+} // namespace internal
+
+} // namespace scopes
+
+} // namespace unity
+
+#endif

=== modified file 'src/scopes/internal/ActivationQueryObject.cpp'
--- src/scopes/internal/ActivationQueryObject.cpp	2014-02-27 23:29:10 +0000
+++ src/scopes/internal/ActivationQueryObject.cpp	2014-03-06 09:05:05 +0000
@@ -21,7 +21,7 @@
 #include <unity/scopes/ActivationQueryBase.h>
 #include <unity/scopes/internal/MWReply.h>
 #include <unity/scopes/internal/MWQueryCtrl.h>
-#include <iostream>
+#include <unity/scopes/internal/Logging.h>
 #include <cassert>
 
 using namespace std;
@@ -69,13 +69,13 @@
     {
         // TODO: log error
         reply_->finished(ListenerBase::Error, e.what());     // Oneway, can't block
-        cerr << "ActivationQueryObject::run(): " << e.what() << endl;
+        errlog << "ActivationQueryObject::run(): " << e.what();
     }
     catch (...)
     {
         // TODO: log error
         reply_->finished(ListenerBase::Error, "unknown exception");     // Oneway, can't block
-        cerr << "ActivationQueryObject::run(): unknown exception" << endl;
+        errlog << "ActivationQueryObject::run(): unknown exception";
     }
 }
 

=== modified file 'src/scopes/internal/AnnotationImpl.cpp'
--- src/scopes/internal/AnnotationImpl.cpp	2014-02-27 16:58:50 +0000
+++ src/scopes/internal/AnnotationImpl.cpp	2014-03-06 09:05:05 +0000
@@ -16,9 +16,9 @@
  */
 
 #include <unity/scopes/internal/AnnotationImpl.h>
+#include <unity/scopes/internal/Logging.h>
 #include <unity/UnityExceptions.h>
 #include <sstream>
-#include <iostream>
 #include <cassert>
 
 namespace unity
@@ -85,7 +85,7 @@
 {
     if (annotation_type_ != Annotation::Type::GroupedLink)
     {
-        std::cerr << "Annotation::set_label(): label is allowed in GroupedLink only" << std::endl;
+        errlog << "Annotation::set_label(): label is allowed in GroupedLink only";
     }
     label_ = label;
 }
@@ -94,7 +94,7 @@
 {
     if (annotation_type_ != Annotation::Type::Link)
     {
-        std::cerr << "Annotation::set_icon(): icon is allowed in Link annotations only" << std::endl;
+        errlog << "Annotation::set_icon(): icon is allowed in Link annotations only";
     }
     icon_ = icon;
 }
@@ -112,7 +112,7 @@
 {
     if (annotation_type_ != Annotation::Type::GroupedLink)
     {
-        std::cerr << "Annotation::label(): label is allowed in GroupedLink only" << std::endl;
+        errlog << "Annotation::label(): label is allowed in GroupedLink only";
     }
     return label_;
 }
@@ -121,7 +121,7 @@
 {
     if (annotation_type_ != Annotation::Type::Link)
     {
-        std::cerr << "Annotation::icon(): icon is allowed in Link annotations only" << std::endl;
+        errlog << "Annotation::icon(): icon is allowed in Link annotations only";
     }
     return icon_;
 }

=== modified file 'src/scopes/internal/CMakeLists.txt'
--- src/scopes/internal/CMakeLists.txt	2014-03-04 05:54:24 +0000
+++ src/scopes/internal/CMakeLists.txt	2014-03-06 09:05:05 +0000
@@ -25,6 +25,7 @@
     ${CMAKE_CURRENT_SOURCE_DIR}/FilterStateImpl.cpp
     ${CMAKE_CURRENT_SOURCE_DIR}/JsonCppNode.cpp
     ${CMAKE_CURRENT_SOURCE_DIR}/LinkImpl.cpp
+    ${CMAKE_CURRENT_SOURCE_DIR}/Logging.cpp
     ${CMAKE_CURRENT_SOURCE_DIR}/MiddlewareBase.cpp
     ${CMAKE_CURRENT_SOURCE_DIR}/MiddlewareFactory.cpp
     ${CMAKE_CURRENT_SOURCE_DIR}/MWObject.cpp

=== added file 'src/scopes/internal/Logging.cpp'
--- src/scopes/internal/Logging.cpp	1970-01-01 00:00:00 +0000
+++ src/scopes/internal/Logging.cpp	2014-03-06 09:05:05 +0000
@@ -0,0 +1,119 @@
+/*
+ * Copyright (C) 2014 Canonical Ltd
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License version 3 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Jussi Pakkanen <[e-mail]>
+ */
+
+#include <unity/scopes/internal/Logging.h>
+#include <mutex>
+#include <iostream>
+
+namespace unity
+{
+
+namespace scopes
+{
+
+namespace internal
+{
+
+static std::mutex print_mutex;
+
+LogStream errlog;
+
+LogStream::LogStream() {
+
+}
+
+LogGatherer LogStream::operator<<(const std::string &s)
+{
+    LogGatherer l;
+    l << s;
+    return l;
+}
+
+LogGatherer LogStream::operator<<(const int i)
+{
+    LogGatherer l;
+    l << i;
+    return l;
+}
+
+LogGatherer LogStream::operator<<(const bool i)
+{
+    LogGatherer l;
+    l << i;
+    return l;
+}
+
+LogGatherer LogStream::operator<<(const char *msg) {
+    LogGatherer l;
+    l << msg;
+    return l;
+}
+
+LogGatherer::LogGatherer()
+{
+
+}
+
+LogGatherer::~LogGatherer()
+{
+    std::unique_lock<std::mutex> l(print_mutex);
+    if(strings.empty())
+        return;
+    try
+    {
+        for(const auto &i : strings)
+        {
+            std::cerr << i;
+        }
+        const std::string &last = strings.back();
+        if(last.empty() or last.back() != '\n')
+        {
+            std::cerr << std::endl;
+        }
+    } catch(...) {
+        // There's not much we can do in this case.
+    }
+}
+
+LogGatherer& LogGatherer::operator<<(const std::string &s)
+{
+    strings.push_back(s);
+    return *this;
+}
+
+LogGatherer& LogGatherer::operator<<(const int i)
+{
+    return *this << std::to_string(i);
+}
+
+LogGatherer& LogGatherer::operator<<(const bool i)
+{
+    std::string val(i ? "true" : "false");
+    return *this << val;
+}
+
+LogGatherer& LogGatherer::operator<<(const char *msg) {
+    strings.emplace_back(msg);
+    return *this;
+}
+
+} // namespace internal
+
+} // namespace scopes
+
+} // namespace unity

=== modified file 'src/scopes/internal/PreviewQueryObject.cpp'
--- src/scopes/internal/PreviewQueryObject.cpp	2014-03-04 03:15:11 +0000
+++ src/scopes/internal/PreviewQueryObject.cpp	2014-03-06 09:05:05 +0000
@@ -16,6 +16,7 @@
  * Authored by: Michal Hruby <[e-mail]>
 */
 
+#include <unity/scopes/internal/Logging.h>
 #include <unity/scopes/internal/MWQueryCtrl.h>
 #include <unity/scopes/internal/MWReply.h>
 #include <unity/scopes/internal/PreviewQueryObject.h>
@@ -25,8 +26,9 @@
 #include <unity/scopes/ListenerBase.h>
 #include <unity/scopes/QueryBase.h>
 #include <unity/scopes/ReplyProxyFwd.h>
+#include <unity/scopes/SearchReply.h>
+#include <unity/scopes/PreviewReply.h>
 
-#include <iostream>
 #include <cassert>
 
 using namespace std;
@@ -79,13 +81,13 @@
         pushable_ = false;
         // TODO: log error
         reply_->finished(ListenerBase::Error, e.what());     // Oneway, can't block
-        cerr << "PreviewQueryObject::run(): " << e.what() << endl;
+        errlog << "PreviewQueryObject::run(): " << e.what();
     }
     catch (...)
     {
         // TODO: log error
         reply_->finished(ListenerBase::Error, "unknown exception");     // Oneway, can't block
-        cerr << "PreviewQueryObject::run(): unknown exception" << endl;
+        errlog << "PreviewQueryObject::run(): unknown exception";
     }
 }
 

=== modified file 'src/scopes/internal/QueryBaseImpl.cpp'
--- src/scopes/internal/QueryBaseImpl.cpp	2014-02-27 23:00:59 +0000
+++ src/scopes/internal/QueryBaseImpl.cpp	2014-03-06 09:05:05 +0000
@@ -17,12 +17,12 @@
  */
 
 #include <unity/scopes/internal/QueryBaseImpl.h>
+#include <unity/scopes/internal/Logging.h>
 
 #include <unity/scopes/QueryCtrl.h>
 #include <unity/scopes/Scope.h>
 #include <unity/scopes/SearchMetadata.h>
 
-#include <iostream>
 #include <cassert>
 
 using namespace std;
@@ -105,7 +105,7 @@
     catch (std::bad_cast const& e) // this shouldn't really happen, if it does, that's a bug
     {
         // TODO: log this
-        std::cerr << "QueryBaseImpl()::set_metadata(): " << e.what() << std::endl;
+        errlog << "QueryBaseImpl()::set_metadata(): " << e.what();
         throw;
     }
 }

=== modified file 'src/scopes/internal/QueryCtrlImpl.cpp'
--- src/scopes/internal/QueryCtrlImpl.cpp	2014-01-23 11:28:34 +0000
+++ src/scopes/internal/QueryCtrlImpl.cpp	2014-03-06 09:05:05 +0000
@@ -18,6 +18,7 @@
 
 #include <unity/scopes/internal/QueryCtrlImpl.h>
 
+#include <unity/scopes/internal/Logging.h>
 #include <unity/scopes/internal/MiddlewareBase.h>
 #include <unity/scopes/internal/MWQueryCtrl.h>
 #include <unity/scopes/internal/MWReply.h>
@@ -26,7 +27,6 @@
 #include <unity/scopes/ScopeExceptions.h>
 
 #include <cassert>
-#include <iostream> // TODO: remove this once logging is added
 
 using namespace std;
 
@@ -67,7 +67,7 @@
     }
     catch (std::exception const& e)
     {
-        cerr << e.what() << endl;
+        errlog << e.what();
         // TODO: log error
     }
 }

=== modified file 'src/scopes/internal/QueryObject.cpp'
--- src/scopes/internal/QueryObject.cpp	2014-03-04 03:15:11 +0000
+++ src/scopes/internal/QueryObject.cpp	2014-03-06 09:05:05 +0000
@@ -18,6 +18,7 @@
 
 #include <unity/scopes/internal/QueryObject.h>
 
+#include <unity/scopes/internal/Logging.h>
 #include <unity/Exception.h>
 #include <unity/scopes/ActivationQueryBase.h>
 #include <unity/scopes/internal/MWQueryCtrl.h>
@@ -29,7 +30,6 @@
 #include <unity/scopes/QueryBase.h>
 #include <unity/scopes/SearchQueryBase.h>
 
-#include <iostream>
 #include <cassert>
 
 using namespace std;
@@ -116,14 +116,14 @@
         pushable_ = false;
         // TODO: log error
         reply_->finished(ListenerBase::Error, e.what());     // Oneway, can't block
-        cerr << "ScopeBase::run(): " << e.what() << endl;
+        errlog << "ScopeBase::run(): " << e.what();
     }
     catch (...)
     {
         pushable_ = false;
         // TODO: log error
         reply_->finished(ListenerBase::Error, "unknown exception");     // Oneway, can't block
-        cerr << "ScopeBase::run(): unknown exception" << endl;
+        errlog << "ScopeBase::run(): unknown exception";
     }
 }
 

=== modified file 'src/scopes/internal/ReplyImpl.cpp'
--- src/scopes/internal/ReplyImpl.cpp	2014-03-04 06:26:21 +0000
+++ src/scopes/internal/ReplyImpl.cpp	2014-03-06 09:05:05 +0000
@@ -16,6 +16,7 @@
  * Authored by: Michi Henning <[e-mail]>
  */
 
+#include <unity/scopes/internal/Logging.h>
 #include <unity/scopes/internal/ReplyImpl.h>
 
 #include <unity/scopes/Annotation.h>
@@ -37,7 +38,7 @@
 
 #include <cassert>
 #include <sstream>
-#include <iostream> // TODO: remove this once logging is added
+#include <cassert>
 
 using namespace std;
 
@@ -262,7 +263,7 @@
         catch (std::exception const& e)
         {
             // TODO: log error
-            cerr << e.what() << endl;
+            errlog << e.what();
         }
     }
 }
@@ -290,7 +291,7 @@
         error_message = "unknown exception";
     }
     // TODO: log error
-    cerr << error_message << endl;
+    errlog << error_message;
 
     try
     {
@@ -299,7 +300,7 @@
     catch (std::exception const& e)
     {
         // TODO: log error
-        cerr << e.what() << endl;
+        errlog << e.what();
     }
 }
 

=== modified file 'src/scopes/internal/ReplyObject.cpp'
--- src/scopes/internal/ReplyObject.cpp	2014-02-24 05:04:11 +0000
+++ src/scopes/internal/ReplyObject.cpp	2014-03-06 09:05:05 +0000
@@ -16,6 +16,7 @@
  * Authored by: Michi Henning <[e-mail]>
  */
 
+#include <unity/scopes/internal/Logging.h>
 #include <unity/scopes/internal/ReplyObject.h>
 #include <unity/scopes/internal/ResultReplyObject.h>
 #include <unity/scopes/internal/PreviewReplyObject.h>
@@ -27,7 +28,6 @@
 #include <unity/scopes/internal/CategorisedResultImpl.h>
 
 #include <cassert>
-#include <iostream> // TODO: remove this once logging is added
 
 using namespace std;
 using namespace unity::scopes::internal;
@@ -50,8 +50,8 @@
     assert(receiver_base);
     assert(runtime);
     reap_item_ = runtime->reply_reaper()->add([this] {
-        cerr << "No activity on ReplyObject for scope " << this->origin_proxy_
-             << ": ReplyObject destroyed" << endl;
+        errlog << "No activity on ReplyObject for scope " << this->origin_proxy_
+               << ": ReplyObject destroyed";
         this->disconnect();
     });
 }
@@ -162,12 +162,12 @@
     }
     catch (std::exception const& e)
     {
-        cerr << "ReplyObject::finished(): " << e.what() << endl;
+        errlog << "ReplyObject::finished(): " << e.what();
         // TODO: log error
     }
     catch (...)
     {
-        cerr << "ReplyObject::finished(): unknown exception" << endl;
+        errlog << "ReplyObject::finished(): unknown exception";
         // TODO: log error
     }
 }

=== modified file 'src/scopes/internal/RuntimeImpl.cpp'
--- src/scopes/internal/RuntimeImpl.cpp	2014-02-21 05:12:05 +0000
+++ src/scopes/internal/RuntimeImpl.cpp	2014-03-06 09:05:05 +0000
@@ -19,6 +19,7 @@
 #include <unity/scopes/internal/RuntimeImpl.h>
 
 #include <unity/scopes/internal/DfltConfig.h>
+#include <unity/scopes/internal/Logging.h>
 #include <unity/scopes/internal/RegistryConfig.h>
 #include <unity/scopes/internal/RegistryImpl.h>
 #include <unity/scopes/internal/RuntimeConfig.h>
@@ -30,7 +31,6 @@
 
 #include <cassert>
 #include <future>
-#include <iostream> // TODO: remove this once logging is added
 
 
 using namespace std;
@@ -72,7 +72,7 @@
 
         if (registry_configfile_.empty() || registry_identity_.empty())
         {
-            cerr << "Warning: no registry configured" << endl;
+            errlog << "Warning: no registry configured";
             registry_identity_ = "";
         }
         else
@@ -100,12 +100,12 @@
     }
     catch (std::exception const& e) // LCOV_EXCL_LINE
     {
-        cerr << "~RuntimeImpl(): " << e.what() << endl;
+        errlog << "~RuntimeImpl(): " << e.what();
         // TODO: log error
     }
     catch (...) // LCOV_EXCL_LINE
     {
-        cerr << "~RuntimeImpl(): unknown exception" << endl;
+        errlog << "~RuntimeImpl(): unknown exception";
         // TODO: log error
     }
 }

=== modified file 'src/scopes/internal/ScopeImpl.cpp'
--- src/scopes/internal/ScopeImpl.cpp	2014-02-27 23:00:59 +0000
+++ src/scopes/internal/ScopeImpl.cpp	2014-03-06 09:05:05 +0000
@@ -18,6 +18,7 @@
 
 #include <unity/scopes/internal/ScopeImpl.h>
 
+#include <unity/scopes/internal/Logging.h>
 #include <unity/scopes/internal/ResultImpl.h>
 #include <unity/scopes/internal/MiddlewareBase.h>
 #include <unity/scopes/internal/MWScope.h>
@@ -33,7 +34,6 @@
 #include <unity/scopes/internal/PreviewReplyObject.h>
 
 #include <cassert>
-#include <iostream> // TODO: remove this once logging is added
 
 using namespace std;
 
@@ -134,7 +134,7 @@
     catch (std::exception const& e)
     {
         // TODO: log error
-        cerr << "activate(): " << e.what() << endl;
+        errlog << "activate(): " << e.what();
         try
         {
             ro->finished(ListenerBase::Error, e.what());
@@ -142,7 +142,7 @@
         }
         catch (...)
         {
-            cerr << "activate(): unknown exception" << endl;
+            errlog << "activate(): unknown exception";
         }
         throw;
     }
@@ -172,7 +172,7 @@
     catch (std::exception const& e)
     {
         // TODO: log error
-        cerr << "perform_action(): " << e.what() << endl;
+        errlog << "perform_action(): " << e.what();
         try
         {
             // TODO: if things go wrong, we need to make sure that the reply object
@@ -182,7 +182,7 @@
         }
         catch (...)
         {
-            cerr << "perform_action(): unknown exception" << endl;
+            errlog << "perform_action(): unknown exception";
         }
         throw;
     }
@@ -216,7 +216,7 @@
     catch (std::exception const& e)
     {
         // TODO: log error
-        cerr << "preview(): " << e.what() << endl;
+        errlog << "preview(): " << e.what();
         try
         {
             ro->finished(ListenerBase::Error, e.what());
@@ -224,7 +224,7 @@
         }
         catch (...)
         {
-            cerr << "preview(): unknown exception" << endl;
+            errlog << "preview(): unknown exception";
         }
         throw;
     }

=== modified file 'src/scopes/internal/ScopeObject.cpp'
--- src/scopes/internal/ScopeObject.cpp	2014-02-27 23:29:10 +0000
+++ src/scopes/internal/ScopeObject.cpp	2014-03-06 09:05:05 +0000
@@ -19,6 +19,7 @@
 #include <unity/scopes/internal/ScopeObject.h>
 
 #include <unity/scopes/internal/ActivationQueryObject.h>
+#include <unity/scopes/internal/Logging.h>
 #include <unity/scopes/internal/MWQuery.h>
 #include <unity/scopes/internal/MWReply.h>
 #include <unity/scopes/internal/PreviewQueryObject.h>
@@ -29,7 +30,6 @@
 #include <unity/UnityExceptions.h>
 
 #include <cassert>
-#include <iostream> // TODO: remove this once logging is added
 #include <sstream>
 
 using namespace std;
@@ -122,7 +122,7 @@
         catch (...)
         {
         }
-        cerr << "query(): " << e.what() << endl;
+        errlog << "query(): " << e.what();
         // TODO: log error
         throw;
     }
@@ -135,7 +135,7 @@
         catch (...)
         {
         }
-        cerr << "query(): unknown exception" << endl;
+        errlog << "query(): unknown exception";
         // TODO: log error
         throw;
     }

=== modified file 'test/gtest/scopes/internal/CMakeLists.txt'
--- test/gtest/scopes/internal/CMakeLists.txt	2014-02-24 14:56:26 +0000
+++ test/gtest/scopes/internal/CMakeLists.txt	2014-03-06 09:05:05 +0000
@@ -3,6 +3,7 @@
 add_subdirectory(DynamicLoader)
 add_subdirectory(JsonNode)
 add_subdirectory(lttng)
+add_subdirectory(Logging)
 add_subdirectory(MiddlewareFactory)
 add_subdirectory(Reaper)
 add_subdirectory(RegistryConfig)

=== added directory 'test/gtest/scopes/internal/Logging'
=== added file 'test/gtest/scopes/internal/Logging/CMakeLists.txt'
--- test/gtest/scopes/internal/Logging/CMakeLists.txt	1970-01-01 00:00:00 +0000
+++ test/gtest/scopes/internal/Logging/CMakeLists.txt	2014-03-06 09:05:05 +0000
@@ -0,0 +1,14 @@
+add_executable(
+    Logging_test
+    Logging_test.cpp
+)
+
+target_link_libraries(
+    Logging_test
+    ${TESTLIBS}
+)
+
+add_test(
+    Logging
+    Logging_test
+)

=== added file 'test/gtest/scopes/internal/Logging/Logging_test.cpp'
--- test/gtest/scopes/internal/Logging/Logging_test.cpp	1970-01-01 00:00:00 +0000
+++ test/gtest/scopes/internal/Logging/Logging_test.cpp	2014-03-06 09:05:05 +0000
@@ -0,0 +1,53 @@
+/*
+ * Copyright © 2014 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU Lesser General Public License version 3,
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Jussi Pakkanen <[e-mail]>
+ */
+
+#include <unity/scopes/internal/Logging.h>
+#include <unity/UnityExceptions.h>
+#include <unity/scopes/Variant.h>
+
+#include <gtest/gtest.h>
+#include <memory>
+#include <algorithm>
+
+using namespace testing;
+using unity::scopes::internal::errlog;
+
+namespace
+{
+
+class LoggingTest : public Test
+{
+public:
+    LoggingTest()
+    {
+    }
+
+};
+
+TEST_F(LoggingTest, basic)
+{
+    std::string msg("Second message to stderr.");
+    errlog << "First message to stderr.";
+    errlog << msg;
+    errlog << "Piecewise " << "message " << "to stderr.\n";
+    errlog << 1 << " is the loneliest number. Not " << 2 << ".\n";
+    errlog << "True is " << true << " and False is " << false << ".";
+}
+
+
+} // namespace