-
Notifications
You must be signed in to change notification settings - Fork 36
Conversation
raft/raft.cpp
Outdated
|
||
for (const auto& peer : this->peers) { | ||
auto ep = boost::asio::ip::tcp::endpoint{boost::asio::ip::address_v4::from_string(peer.host), peer.port}; | ||
this->node->send_message(ep, json_ptr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't bother with the temporary and construct and endpoint in the send_message call.
|
||
void | ||
audit::start() | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like wrapping this in a lambda called by std::call_once
audit/audit.cpp
Outdated
using namespace bzn; | ||
|
||
audit::audit(std::shared_ptr<bzn::node_base> node): node(node) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd format this as:
audit::audit(...)
: node(node)
{
}
a bit easier to read
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please also use std::move(node)
audit/audit.cpp
Outdated
return this->recorded_errors; | ||
} | ||
|
||
uint |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please use size_t
audit/audit.hpp
Outdated
const std::list<std::string>& error_strings() const override; | ||
|
||
void handle(const bzn::message& message, std::shared_ptr<bzn::session_base> session) override; | ||
void handle_commit(const commit_notification&); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't these also be in audit_base?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My not need the abstract base class if we don't intend to mock this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few things...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remember to change the commit message to follow our standard format: KEP-368 <title>
First commit is the changes from last time squashed, second is changes since then.