KEP-574: changes from code review (EB)

pbft/pbft.cpp

@@ -730,7 +730,7 @@ pbft::initialize_configuration(const bzn::peers_list_t& peers)
 {
     auto config = std::make_shared<pbft_configuration>();
     bool config_good = true;
-    for (auto p : peers)
+    for (auto& p : peers)
     {
         config_good &= config->add_peer(p);
     }
@@ -752,7 +752,9 @@ pbft::current_peers_ptr() const
 {
     auto config = this->configurations.current();
     if (config)
+    {
         return config->get_peers();
+    }
 
     throw std::runtime_error("No current configuration!");
 }

pbft/pbft_config_store.cpp

@@ -16,11 +16,6 @@
 
 using namespace bzn;
 
-pbft_config_store::pbft_config_store()
-    : current_index(0), next_index(1)
-{
-}
-
 bool
 pbft_config_store::add(pbft_configuration::shared_const_ptr config)
 {
@@ -46,7 +41,9 @@ pbft_config_store::set_current(const hash_t& hash)
 {
     auto config = this->find_by_hash(hash);
     if (config == this->configs.end())
+    {
         return false;
+    }
 
     this->current_index = config->first;
     return true;
@@ -57,7 +54,9 @@ pbft_config_store::remove_prior_to(const hash_t& hash)
 {
     auto config = this->find_by_hash(hash);
     if (config == this->configs.end())
+    {
         return false;
+    }
 
     this->configs.erase(this->configs.begin(), config);
     return true;
@@ -75,7 +74,7 @@ pbft_config_store::enable(const hash_t& hash, bool val)
 {
     // can't find_by_hash here because we need a non-const
     auto config = std::find_if(this->configs.begin(), this->configs.end(),
-        [hash](auto c)
+        [hash](auto& c)
         {
             return c.second.first->get_hash() == hash;
         });

pbft/pbft_config_store.hpp

@@ -24,8 +24,6 @@ namespace bzn
     class pbft_config_store
     {
     public:
-        pbft_config_store();
-
         bool add(pbft_configuration::shared_const_ptr config);
         bool remove_prior_to(const hash_t& hash);
         pbft_configuration::shared_const_ptr get(const hash_t& hash) const;
@@ -43,8 +41,8 @@ namespace bzn
 
         // a map from the config index to a pair of <config, is_config_enabled>
         config_map configs;
-        index_t current_index;
-        index_t next_index;
+        index_t current_index = 0;
+        index_t next_index = 1;
     };
 }
 

pbft/pbft_configuration.cpp

@@ -50,7 +50,7 @@ pbft_configuration::from_json(const bzn::json_message& json)
 
     bool result = true;
     this->peers.clear();
-    for (auto p : json["peers"])
+    for (auto& p : json["peers"])
     {
         bzn::peer_address_t peer(p["host"].asString(), static_cast<uint16_t>(p["port"].asUInt()),
             static_cast<uint16_t>(p["http_port"].asUInt()), p["name"].asString(), p["uuid"].asString());
@@ -96,9 +96,7 @@ pbft_configuration::create_hash() const
 {
     // TODO: better hash function
 
-    auto json = this->to_json().toStyledString();
-    size_t h = std::hash<std::string>{}(json);
-    return std::to_string(h);
+    return std::to_string(std::hash<std::string>{}(this->to_json().toStyledString()));
 }
 
 hash_t
@@ -180,22 +178,26 @@ pbft_configuration::cache_sorted_peers()
         }
     );
 
-    this->sorted_peers = sorted;
+    this->sorted_peers = std::move(sorted);
     this->cached_hash = this->create_hash();
 }
 
 bool
 pbft_configuration::conflicting_peer_exists(const bzn::peer_address_t &peer) const
 {
-    for (auto p : this->peers)
+    for (auto& p : this->peers)
     {
         if (p.uuid == peer.uuid || p.name == peer.name)
+        {
             return true;
+        }
 
         if (p.host == peer.host)
         {
             if (p.port == peer.port || p.http_port == peer.http_port)
+            {
                 return true;
+            }
         }
     }
 
@@ -206,7 +208,9 @@ bool
 pbft_configuration::valid_peer(const bzn::peer_address_t &peer) const
 {
     if (peer.name.empty() || peer.uuid.empty() || peer.host.empty() || !peer.port || !peer.http_port)
+    {
         return false;
+    }
 
     // TODO: validate host address?