tests modified accordingly to unique_ptrs

src/TinyMqtt.cpp

@@ -17,21 +17,12 @@ int TinyMqtt::debug=2;
 
 MqttBroker::MqttBroker(uint16_t port)
 {
-  server = new TcpServer(port);
+  server = std::unique_ptr<TcpServer>(new TcpServer(port));
 #ifdef TINY_MQTT_ASYNC
   server->onClient(onClient, this);
 #endif
 }
 
-MqttBroker::~MqttBroker()
-{
-  while(clients.size())
-  {
-    delete clients[0];
-  }
-  delete server;
-}
-
 // private constructor used by broker only
 MqttClient::MqttClient(MqttBroker* local_broker, TcpClient* new_client)
 {
@@ -43,7 +34,7 @@ MqttClient::MqttClient(MqttBroker* local_broker, TcpClient* new_client)
   // client->onConnect() TODO
   // client->onDisconnect() TODO
 #else
-  tcp_client = new WiFiClient(*new_client);
+  tcp_client.reset(new WiFiClient(*new_client));
 #endif
   alive = millis()+5000;
 }
@@ -59,7 +50,6 @@ MqttClient::MqttClient(MqttBroker* local_broker, const std::string& id)
 MqttClient::~MqttClient()
 {
   close();
-  delete tcp_client;
   debug("*** MqttClient delete()");
 }
 
@@ -99,8 +89,7 @@ void MqttClient::connect(std::string broker, uint16_t port, uint16_t ka)
   debug("MqttClient::connect_to_host " << broker << ':' << port);
   keep_alive = ka;
   close();
-  if (tcp_client) delete tcp_client;
-  tcp_client = new TcpClient;
+  tcp_client.reset(new TcpClient);
 
 #ifdef TINY_MQTT_ASYNC
   tcp_client->onData(onData, this);
@@ -110,7 +99,7 @@ void MqttClient::connect(std::string broker, uint16_t port, uint16_t ka)
   if (tcp_client->connect(broker.c_str(), port))
   {
     debug("link established");
-    onConnect(this, tcp_client);
+    onConnect(this, tcp_client.get());
   }
   else
   {
@@ -119,10 +108,10 @@ void MqttClient::connect(std::string broker, uint16_t port, uint16_t ka)
 #endif
 }
 
-void MqttBroker::addClient(MqttClient* client)
+void MqttBroker::addClient(TcpClient* client)
 {
   debug("MqttBroker::addClient");
-  clients.push_back(client);
+  clients.insert(std::unique_ptr<MqttClient>(new MqttClient(this, client)));
 }
 
 void MqttBroker::connect(const std::string& host, uint16_t port)
@@ -135,24 +124,7 @@ void MqttBroker::connect(const std::string& host, uint16_t port)
 
 void MqttBroker::removeClient(MqttClient* remove)
 {
-  debug("removeClient");
-  for(auto it=clients.begin(); it!=clients.end(); it++)
-  {
-    auto client=*it;
-    if (client==remove)
-    {
-      // TODO if this broker is connected to an external broker
-      // we have to unsubscribe remove's topics.
-      // (but doing this, check that other clients are not subscribed...)
-      // Unless -> we could receive useless messages
-      //        -> we are using (memory) one IndexedString plus its string for nothing.
-      debug("Remove " << clients.size());
-      clients.erase(it);
-      debug("Client removed " << clients.size());
-      return;
-    }
-  }
-  debug(red << "Error cannot remove client");  // TODO should not occur
+  local_clients.erase(remove);
 }
 
 void MqttBroker::onClient(void* broker_ptr, TcpClient* client)
@@ -160,7 +132,7 @@ void MqttBroker::onClient(void* broker_ptr, TcpClient* client)
   debug("MqttBroker::onClient");
   MqttBroker* broker = static_cast<MqttBroker*>(broker_ptr);
 
-  broker->addClient(new MqttClient(broker, client));
+  broker->addClient(client);
   debug("New client");
 }
 
@@ -181,21 +153,18 @@ void MqttBroker::loop()
     remote_broker->loop();
   }
 
-  for(size_t i=0; i<clients.size(); i++)
+  // 200 bytes shorter than for(auto& client: clients) !
+  for(auto it=clients.begin(); it!=clients.end(); it++)
   {
-    MqttClient* client = clients[i];
-    if (client->connected())
+    it->get()->loop();
+    if (not it->get()->connected())
     {
-      client->loop();
-    }
-    else
-    {
-      debug("Client " << client->id().c_str() << "  Disconnected, local_broker=" << (dbg_ptr)client->local_broker);
-      // Note: deleting a client not added by the broker itself will probably crash later.
-      delete client;
+      clients.erase(it);
       break;
     }
   }
+  for(const auto& client: local_clients)
+    client->loop();
 }
 
 MqttError MqttBroker::subscribe(const Topic& topic, uint8_t qos)
@@ -213,14 +182,13 @@ MqttError MqttBroker::publish(const MqttClient* source, const Topic& topic, Mqtt
   MqttError retval = MqttOk;
 
   debug("MqttBroker::publish");
-  int i=0;
-  for(auto client: clients)
+  int clt_num = 0;
+  for(auto& client: clients)
   {
-    i++;
-#if TINY_MQTT_DEBUG
-    Console << __LINE__ << " broker:" << (remote_broker && remote_broker->connected() ? "linked" : "alone") <<
-       "  srce=" << (source->isLocal() ? "loc" : "rem") << " clt#" << i << ", local=" << client->isLocal() << ", con=" << client->connected() << endl;
-#endif
+    debug (" broker:" << (remote_broker && remote_broker->connected() ? "linked" : "alone")
+        << "  srce=" << (source->isLocal() ? "loc" : "rem") << " clt#" << ++clt_num
+        << ", local=" << client->isLocal() << ", con=" << client->connected());
+
     bool doit = false;
     if (remote_broker && remote_broker->connected())  // this (MqttBroker) is connected (to a external broker)
     {
@@ -238,9 +206,8 @@ MqttError MqttBroker::publish(const MqttClient* source, const Topic& topic, Mqtt
     {
       doit = true;
     }
-#if TINY_MQTT_DEBUG
-    Console << ", doit=" << doit << ' ';
-#endif
+
+    debug(" doit=" << doit << ' ');
 
     if (doit) retval = client->publishIfSubscribed(topic, msg);
     debug("");
@@ -359,7 +326,7 @@ void MqttClient::resubscribe()
     msg.add(0);
     msg.add(0);
 
-    for(auto topic: subscriptions)
+    for(const auto& topic: subscriptions)
     {
       msg.add(topic);
       msg.add(0);    // TODO qos
@@ -481,9 +448,8 @@ void MqttClient::processMessage(MqttMessage* mesg)
         payload += len;
       }
 
-      #if TINY_MQTT_DEBUG
-        Console << yellow << "Client " << clientId << " connected : keep alive=" << keep_alive << '.' << white << endl;
-      #endif
+      debug(yellow << "Client " << clientId << " connected : keep alive=" << keep_alive << '.' << white);
+
       bclose = false;
       mqtt_flags |= FlagConnected;
       {
@@ -579,9 +545,7 @@ void MqttClient::processMessage(MqttMessage* mesg)
       break;
 
     case MqttMessage::Type::Publish:
-      #if TINY_MQTT_DEBUG
-        Console << "publish " << (mqtt_flags & FlagConnected) << '/' << (long) tcp_client << endl;
-      #endif
+      debug("publish " << (mqtt_flags & FlagConnected) << '/' << (long) tcp_client.get());
       if ((mqtt_flags & FlagConnected) or tcp_client == nullptr)
       {
         uint8_t qos = mesg->flags();
@@ -589,9 +553,7 @@ void MqttClient::processMessage(MqttMessage* mesg)
         mesg->getString(payload, len);
         Topic published(payload, len);
         payload += len;
-        #if TINY_MQTT_DEBUG
-          Console << "Received Publish (" << published.str().c_str() << ") size=" << (int)len << endl;
-        #endif
+        debug("Received Publish (" << published.str().c_str() << ") size=" << (int)len);
         // << '(' << std::string(payload, len).c_str() << ')'  << " msglen=" << mesg->length() << endl;
         if (qos) payload+=2;  // ignore packet identifier if any
         len=mesg->end()-payload;
@@ -743,10 +705,7 @@ MqttError MqttClient::publishIfSubscribed(const Topic& topic, MqttMessage& msg)
     else
     {
       processMessage(&msg);
-
-      #if TINY_MQTT_DEBUG
-        Console << "Should call the callback ?\n";
-      #endif
+      debug("Should call the callback ?");
       // callback(this, topic, nullptr, 0);  // TODO Payload
     }
   }

src/TinyMqtt.h

@@ -37,7 +37,7 @@
   #include <rpcWiFi.h>
 #endif
 
-#include <vector>
+#include <memory>
 #include <set>
 #include <string>
 #include "StringIndexer.h"
@@ -52,7 +52,7 @@
     static int debug;
   };
 
-  #define debug(what) { if (TinyMqtt::debug>=1) Console << (int)__LINE__ << ' ' << what << TinyConsole::white << endl; delay(100); }
+  #define debug(what) { if (TinyMqtt::debug>=1) Console << (int)__LINE__ << ' ' << what << TinyConsole::white << endl; delay(10); }
 #else
   #define debug(what) {}
 #endif
@@ -303,7 +303,7 @@ class MqttClient
     // when MqttBroker uses MqttClient for each external connexion
     MqttBroker* local_broker=nullptr;
 
-    TcpClient* tcp_client=nullptr;    // connection to remote broker
+    std::unique_ptr<TcpClient> tcp_client;    // connection to remote broker
     std::set<Topic> subscriptions;
     std::string clientId;
     CallBack callback = nullptr;
@@ -320,7 +320,6 @@ class MqttBroker
   public:
     // TODO limit max number of clients
     MqttBroker(uint16_t port);
-    ~MqttBroker();
 
     void begin() { server->begin(); }
     void loop();
@@ -328,15 +327,21 @@ class MqttBroker
     void connect(const std::string& host, uint16_t port=1883);
     bool connected() const { return state == Connected; }
 
-    size_t clientsCount() const { return clients.size(); }
 
     void dump(std::string indent="")
     {
-      for(auto client: clients)
+      for(const auto& client: clients)
         client->dump(indent);
     }
 
-    const std::vector<MqttClient*>  getClients() const { return clients; }
+    using Clients = std::set<std::unique_ptr<MqttClient>>;
+    using LocalClients = std::set<MqttClient*>;
+
+    const Clients& getClients() const { return clients; }
+    const LocalClients& getLocalClients() const { return local_clients; }
+
+    size_t clientsCount() const { return clients.size(); }
+    size_t localClientsCount() const { return local_clients.size(); }
 
   private:
     friend class MqttClient;
@@ -353,15 +358,17 @@ class MqttBroker
 
     MqttError subscribe(const Topic& topic, uint8_t qos);
 
-    // For clients that are added not by the broker itself (local clients)
-    void addClient(MqttClient* client);
-    void removeClient(MqttClient* client);
+    void addClient(MqttClient* local) { local_clients.insert(local); }
+    void addClient(TcpClient* client);
+
+    void removeClient(MqttClient* local);
 
     bool compareString(const char* good, const char* str, uint8_t str_len) const;
-    std::vector<MqttClient*>  clients;
+    Clients clients;
+    LocalClients local_clients;
 
   private:
-    TcpServer* server = nullptr;
+    std::unique_ptr<TcpServer> server;
 
     const char* auth_user = "guest";
     const char* auth_password = "guest";

src/make_unique.inc

@@ -0,0 +1,142 @@
+// Implementation of C++14's make_unique for C++11 compilers.
+//
+// This has been tested with:
+// - MSVC 11.0 (Visual Studio 2012)
+// - gcc 4.6.3
+// - Xcode 4.4 (with clang "4.0")
+//
+// It is based off an implementation proposed by Stephan T. Lavavej for
+// inclusion in the C++14 standard:
+//    http://isocpp.org/files/papers/N3656.txt
+// Where appropriate, it borrows the use of MSVC's _VARIADIC_EXPAND_0X macro
+// machinery to compensate for lack of variadic templates.
+//
+// This file injects make_unique into the std namespace, which I acknowledge is
+// technically forbidden ([C++11: 17.6.4.2.2.1/1]), but is necessary in order
+// to have syntax compatibility with C++14.
+//
+// I perform compiler version checking for MSVC, gcc, and clang to ensure that
+// we don't add make_unique if it is already there (instead, we include
+// <memory> to get the compiler-provided one). You can override the compiler
+// version checking by defining the symbol COMPILER_SUPPORTS_MAKE_UNIQUE.
+//
+//
+// ===============================================================================
+// This file is released into the public domain. See LICENCE for more information.
+// ===============================================================================
+
+#pragma once
+
+// If user hasn't specified COMPILER_SUPPORTS_MAKE_UNIQUE then try to figure out
+// based on compiler version if std::make_unique is provided.
+#if !defined(COMPILER_SUPPORTS_MAKE_UNIQUE)
+   #if defined(_MSC_VER)
+      // std::make_unique was added in MSVC 12.0
+      #if _MSC_VER >= 1800 // MSVC 12.0 (Visual Studio 2013)
+         #define COMPILER_SUPPORTS_MAKE_UNIQUE
+      #endif
+   #elif defined(__clang__)
+      // std::make_unique was added in clang 3.4, but not until Xcode 6.
+      // Annoyingly, Apple makes the clang version defines match the version
+      // of Xcode, not the version of clang.
+      #define CLANG_VERSION (__clang_major__ * 10000 + __clang_minor__ * 100 + __clang_patchlevel__)
+      #if defined(__APPLE__) && CLANG_VERSION >= 60000
+         #define COMPILER_SUPPORTS_MAKE_UNIQUE
+      #elif !defined(__APPLE__) && CLANG_VERSION >= 30400 
+         #define COMPILER_SUPPORTS_MAKE_UNIQUE
+      #endif
+   #elif defined(__GNUC__)
+      // std::make_unique was added in gcc 4.9, for standards versions greater
+      // than -std=c++11.
+      #define GCC_VERSION (__GNUC__ * 10000 + __GNUC_MINOR__ * 100 + __GNUC_PATCHLEVEL__)
+      #if GCC_VERSION >= 40900 && __cplusplus > 201103L
+         #define COMPILER_SUPPORTS_MAKE_UNIQUE
+      #endif
+   #endif
+#endif
+
+#if defined(COMPILER_SUPPORTS_MAKE_UNIQUE)
+
+// If the compiler supports std::make_unique, then pull in <memory> to get it.
+#include <memory>
+
+#else
+
+// Otherwise, the compiler doesn't provide it, so implement it ourselves.
+
+#include <cstddef>
+#include <memory>
+#include <type_traits>
+#include <utility>
+
+namespace std {
+
+template<class _Ty> struct _Unique_if {
+   typedef unique_ptr<_Ty> _Single_object;
+};
+
+template<class _Ty> struct _Unique_if<_Ty[]> {
+   typedef unique_ptr<_Ty[]> _Unknown_bound;
+};
+
+template<class _Ty, size_t N> struct _Unique_if<_Ty[N]> {
+   typedef void _Known_bound;
+};
+
+//
+// template< class T, class... Args >
+// unique_ptr<T> make_unique( Args&&... args);
+//
+
+#if defined(_MSC_VER) && (_MSC_VER < 1800)
+
+// Macro machinery because MSVC 11.0 doesn't support variadic templates.
+// The _VARIADIC_EXPAND_0X stuff is defined in <xstddef>
+#define _MAKE_UNIQUE( \
+   TEMPLATE_LIST, PADDING_LIST, LIST, COMMA, X1, X2, X3, X4) \
+   template<class _Ty COMMA LIST(_CLASS_TYPE)> inline \
+      typename _Unique_if<_Ty>::_Single_object make_unique(LIST(_TYPE_REFREF_ARG)) \
+   { \
+      return unique_ptr<_Ty>(new _Ty(LIST(_FORWARD_ARG))); \
+   } \
+
+_VARIADIC_EXPAND_0X(_MAKE_UNIQUE, , , , )
+#undef _MAKE_UNIQUE
+
+#else // not MSVC 11.0 or earlier
+
+template<class _Ty, class... Args>
+   typename _Unique_if<_Ty>::_Single_object
+   make_unique(Args&&... args) {
+      return unique_ptr<_Ty>(new _Ty(std::forward<Args>(args)...));
+   }
+
+#endif
+
+// template< class T >
+// unique_ptr<T> make_unique( std::size_t size );
+
+template<class _Ty>
+   typename _Unique_if<_Ty>::_Unknown_bound
+   make_unique(size_t n) {
+      typedef typename remove_extent<_Ty>::type U;
+      return unique_ptr<_Ty>(new U[n]());
+   }
+
+// template< class T, class... Args >
+// /* unspecified */ make_unique( Args&&... args ) = delete;
+
+// MSVC 11.0 doesn't support deleted functions, so the best we can do
+// is simply not define the function.
+#if !(defined(_MSC_VER) && (_MSC_VER < 1800))
+
+template<class T, class... Args>
+   typename _Unique_if<T>::_Known_bound
+   make_unique(Args&&...) = delete;
+
+#endif
+
+} // namespace std
+
+#endif // !COMPILER_SUPPORTS_MAKE_UNIQUE
+

tests/Makefile

@@ -1,4 +1,4 @@
-SUB=
+SUB=n
 
 tests:
 	set -e; \

tests/local-tests/local-tests.ino

@@ -31,13 +31,13 @@ void onPublish(const MqttClient* srce, const Topic& topic, const char* payload,
 test(local_client_should_unregister_when_destroyed)
 {
   MqttBroker broker(1883);
-  assertEqual(broker.clientsCount(), (size_t)0);
+  assertEqual(broker.localClientsCount(), (size_t)0);
   {
-    assertEqual(broker.clientsCount(), (size_t)0);  // Ensure client is not yet connected
+    assertEqual(broker.localClientsCount(), (size_t)0);  // Ensure client is not yet connected
     MqttClient client(&broker);
-    assertEqual(broker.clientsCount(), (size_t)1);  // Ensure client is now connected
+    assertEqual(broker.localClientsCount(), (size_t)1);  // Ensure client is now connected
   }
-  assertEqual(broker.clientsCount(), (size_t)0);
+  assertEqual(broker.localClientsCount(), (size_t)0);
 }
 
 test(local_client_alive)
@@ -47,31 +47,31 @@ test(local_client_alive)
   MqttClient client(&broker);
 
   broker.loop();
-  assertEqual(broker.clientsCount(), (size_t)1);  // Ensure client is now connected
+  assertEqual(broker.localClientsCount(), (size_t)1);  // Ensure client is now connected
 
   add_millis(TINY_MQTT_DEFAULT_ALIVE*1000/2);
   broker.loop();
-  assertEqual(broker.clientsCount(), (size_t)1);  // Ensure client is still connected
+  assertEqual(broker.localClientsCount(), (size_t)1);  // Ensure client is still connected
 
   add_seconds(TINY_MQTT_DEFAULT_ALIVE*5);
   broker.loop();
-  assertEqual(broker.clientsCount(), (size_t)1);  // Ensure client is still connected
+  assertEqual(broker.localClientsCount(), (size_t)1);  // Ensure client is still connected
 }
 
 #if 0
 test(local_connect)
 {
-  assertEqual(broker.clientsCount(), (size_t)0);
+  assertEqual(broker.localClientsCount(), (size_t)0);
 
   MqttClient client;
   assertTrue(client.connected());
-  assertEqual(broker.clientsCount(), (size_t)1);
+  assertEqual(broker.localClientsCount(), (size_t)1);
 }
 
 test(local_publish_should_be_dispatched)
 {
   published.clear();
-  assertEqual(broker.clientsCount(), (size_t)0);
+  assertEqual(broker.localClientsCount(), (size_t)0);
 
   MqttClient subscriber;
   subscriber.subscribe("a/b");
@@ -91,7 +91,7 @@ test(local_publish_should_be_dispatched)
 test(local_publish_should_be_dispatched_to_local_clients)
 {
   published.clear();
-  assertEqual(broker.clientsCount(), (size_t)0);
+  assertEqual(broker.localClientsCount(), (size_t)0);
 
   MqttClient subscriber_a("A");
   subscriber_a.setCallback(onPublish);
@@ -116,7 +116,7 @@ test(local_publish_should_be_dispatched_to_local_clients)
 test(local_unsubscribe)
 {
   published.clear();
-  assertEqual(broker.clientsCount(), (size_t)0);
+  assertEqual(broker.localClientsCount(), (size_t)0);
 
   MqttClient subscriber;
   subscriber.setCallback(onPublish);
@@ -136,7 +136,7 @@ test(local_unsubscribe)
 test(local_nocallback_when_destroyed)
 {
   published.clear();
-  assertEqual(broker.clientsCount(), (size_t)0);
+  assertEqual(broker.localClientsCount(), (size_t)0);
 
   MqttClient publisher;
   {

tests/network-tests/network-tests.ino

@@ -142,6 +142,15 @@ test(suback)
   assertEqual(MqttClient::counters[MqttMessage::Type::SubAck], 1);
 }
 
+uint32_t getClientKeepAlive(MqttBroker& broker)
+{
+  if (broker.getClients().size() == 1)
+    for (auto& it : broker.getClients())
+      return it->keepAlive();
+
+  return 9999;
+}
+
 test(network_client_alive)
 {
   const uint32_t keep_alive=1;
@@ -162,7 +171,7 @@ test(network_client_alive)
   assertTrue(broker.clientsCount() == 1);
   assertTrue(client.connected());
 
-  uint32_t ka = broker.getClients()[0]->keepAlive();
+  uint32_t ka = getClientKeepAlive(broker);
   assertEqual(ka, keep_alive);
   assertEqual(broker.clientsCount(), (size_t)1);
 
@@ -212,9 +221,8 @@ test(network_client_keep_alive_high)
   uint32_t sz = broker.getClients().size();
   assertEqual(sz , (uint32_t)1);
 
-  uint32_t ka = broker.getClients()[0]->keepAlive();
+  uint32_t ka = getClientKeepAlive(broker);
   assertEqual(ka, keep_alive);
-
 }
 
 test(network_client_to_broker_connexion)