Issue #1234, #2058 - Rewrite log facility, improving thread implementation

src/plugin/ActiveQueue.h

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
- * Copyright (C) 2006-2015 Eyeo GmbH

[Oleksandr, 2016/01/28 10:15:34]

Nit: Here and further sadly we have already updated the header to the year 2016

[Eric, 2016/02/04 21:01:44]

Done.

+ * Copyright (C) 2006-2016 Eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus 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 General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 #include <thread>
 #include <mutex>
 #include <condition_variable>
 #include <deque>
 #include "Placeholder.h"
 
 /**
  * A synchronized FIFO queue with thread notice on receipt.
  *
  * \tparam T
  *   Class of the elements of the queue.
  *   It must have a move constructor.
- * \tparam F
- *   Functor type of object to receive notice on Insert().

[Oleksandr, 2016/01/28 10:15:34]

Nit: I guess this parameter was removed at some point?

[Eric, 2016/02/04 21:01:43]

Yes. As I recall, I originally only had one class. I split it into two to
improve testability. This is a leftover.

Fixed.

  */
 template<class T>
 class MessageQueue
 {
   std::deque<T> queue;
   typedef std::lock_guard<std::mutex> SentryType;
   typedef std::unique_lock<std::mutex> UniqueLockType;
   std::mutex mutex;
   std::condition_variable cv;
 public:
   MessageQueue() {} // = default
 
   /*
    * This class does not have any responsibility with regard to the elements of its queue.
    * An instance may be destroyed with elements still in the queue.
    */
   ~MessageQueue() {} // = default;
 
   /**
-   * Insert an element, copy constructor version.

[Oleksandr, 2016/01/28 10:15:34]

Nit: this is not a constructor. I think "lvalue version" and "rvalue version"
explains it better. Or "copy semantics" and "move semantics".

[Eric, 2016/02/04 21:01:44]

OK. I'll use "copy semantics".

+   * Insert an element, copy semantics version.
    */
   void Insert(const T& t)
   {
-    {

[Oleksandr, 2016/01/28 10:15:35]

Just to reiterate Sergei's point - the extra scope is not needed here.

[Eric, 2016/02/04 21:01:44]

Done.

-      SentryType sentry(mutex);
-      queue.push_back(t);
-      cv.notify_one();
-    }
-  }
-
-  /**
-   * Insert an element, move constructor version.
+    SentryType sentry(mutex);
+    queue.push_back(t);
+    cv.notify_one();
+  }
+
+  /**
+   * Insert an element, move semantics version.
    */
   void Insert(T&& t)
   {
-    {
-      SentryType sentry(mutex);
-      queue.push_back(std::move(t));
-      cv.notify_one();
-    }
+    SentryType sentry(mutex);
+    queue.push_back(std::move(t));
+    cv.notify_one();
   }
 
   /**
    * Wake up anything waiting on the condition variable.
    *
    * \tparam Function 
    *   Functor type for argument
    * \param f 
    *   Functor to execute _inside_ the protection of the queue's mutex
    */
   template<typename Function>
   void Rouse(Function& f)
   {
     SentryType sentry(mutex);
     f();
     cv.notify_one();
   }
 
   /**
-   * Remove and Test.
-   * Test that the queue is not empty.
-   * If so, remove the next element from the consumer end of the queue and pass it to the argument function

[Oleksandr, 2016/01/28 10:15:34]

Nit: I think there are too many comments in this function. The next comment
merely describes the workflow. We should aim to make the code readable with
minimal comments, and I think these are redundant. In any case it would be good
to at least have row limit of 80 chars.

[Eric, 2016/02/04 21:01:43]

Reworded.

+   * Test, Remove, and Execute
+   *
+   * If the queue is not empty, remove the next element from the consumer end 
+   *   of the queue and execute the argument functor on it.
    * 
    * \param f
-   *   If return value is true, a functor to which to pass the element at the consumer end of the queue.
+   *   A functor to which to pass the element at the consumer end of the queue.
    *   This functor is executed _outside_ the protection of the queue's mutex.
    * \return
    *   True if and only if an element was removed from the front of the queue.
    */
   template<typename Function>
   bool Remove(Function& f)
   {
     /*
      * We need to remove an element atomically from the queue,
      *   so we need to construct that element _inside_ the protection of the mutex,
-     * On the other hand, we need to execute the function _outside_ the protection of the queue,
-     *   because otherwise performance may suffer.
-     * Hence we use 'Placeholder' so that we can declare a variable to hold the removed element
-     *   without requiring it to have a default constructor or assignment operators.
+     * On the other hand, we need to execute the function _outside_ the protection 
+     *   of the queue, because otherwise performance may suffer.
+     * Hence we use 'Placeholder' so that we can declare a variable to hold the 
+     *   removed element without requiring it to have a default constructor or 
+     *   assignment operators.
      */
     Placeholder<T> x;
     {
       SentryType sentry(mutex);
       if (queue.empty())
         return false;
       x.Construct(std::move(queue.front()));    // only require move-constructibility
       queue.pop_front();
     }
     f(std::move(x.Object()));
@@ skipping 155 matching lines @@
   }
 
   /**
    * Insert
    */
   void Insert(T&& t)
   {
     queue.Insert(std::move(t));
   }
 };