Code review comment for lp:~mhr3/unity/ubus-rewrite

Revision history for this message
Marco Trevisan (TreviƱo) (3v1n0) wrote :

1006 + msg_queue_.insert(std::pair<int, std::pair<std::string, glib::Variant>>(prio, std::pair<std::string, glib::Variant>(message_name, args)));

It can be just std::make_pair(prio, std::make_pair(message_name, args))

1009 + auto src_nick = boost::lexical_cast<std::string>(static_cast<int>(prio));

No need for it with C++11, just use std::to_string(prio); also use auto const& when possible.

985 + for (auto it = interests_.begin(); it != interests_.end(); ++it)

Not tried, but I guess that also the multimaps, would handle nicely a for (auto const& : interests_) foreach loop.

983 +void UBusServer::UnregisterInterest(unsigned connection_id)

I know this is safer, but what's the reason why not to add also UBusServer::UnregisterInterest(std::string const& name) as it used to be?
[NVM, I've noticed that now there's only just one static UBusServer: +1].

1034 + for (unsigned i = 0; i < dispatched_msgs.size(); i++)

Use ++i or just a foreach loop.

1221 + // we don't want to modify a container while iterating it
1222 + std::set<unsigned> ids_copy(connection_ids_);
1223 + for (auto it = ids_copy.begin(); it != ids_copy.end(); ++it)
1224 + {
1225 + UnregisterInterest(*it);
1226 + }

this is also safe:
for (auto it = ids_copy.begin(); it != ids_copy.end();)
{
  UnregisterInterest(*(it++));
}

1317 + static std::unique_ptr<UBusServer> server;

You can just use an stack allocated server here, just remember to add this to the .cpp file:
// initialize the global static server
UBusServer UBusManager::server;

PS: add yourself to to authors, so people will bother you when on troubles ;-D

« Back to merge proposal