diff options
author | Henning Baldersheim <balder@yahoo-inc.com> | 2023-01-31 19:41:08 +0000 |
---|---|---|
committer | Henning Baldersheim <balder@yahoo-inc.com> | 2023-02-01 21:51:45 +0000 |
commit | 79487ed8e135b5948248db91974148d80f2692dc (patch) | |
tree | ef1db9dc8979bd21ca3818d59548b2d39bf1d7e2 | |
parent | 5d4c62ac6a6cc32c9c0893126c603d9428a5dab7 (diff) |
- Deinline destructor.
- Move instead of copy = > noexcept move construtors.
- make_unique/make_shared.
- c++11 for loops
29 files changed, 312 insertions, 535 deletions
diff --git a/documentapi/src/tests/policies/policies_test.cpp b/documentapi/src/tests/policies/policies_test.cpp index 19960d148df..33823681765 100644 --- a/documentapi/src/tests/policies/policies_test.cpp +++ b/documentapi/src/tests/policies/policies_test.cpp @@ -50,18 +50,18 @@ private: const DataType *_docType; private: - bool trySelect(TestFrame &frame, uint32_t numSelects, const std::vector<string> &expected); - void setupExternPolicy(TestFrame &frame, mbus::Slobrok &slobrok, const string &pattern, int32_t numEntries = -1); - ContentPolicy &setupContentPolicy(TestFrame &frame, const string ¶m, - const string &pattern = "", int32_t numEntries = -1); + static bool trySelect(TestFrame &frame, uint32_t numSelects, const std::vector<string> &expected); + static void setupExternPolicy(TestFrame &frame, mbus::Slobrok &slobrok, const string &pattern, int32_t numEntries = -1); + static ContentPolicy &setupContentPolicy(TestFrame &frame, const string ¶m, + const string &pattern = "", int32_t numEntries = -1); bool isErrorPolicy(const string &name, const string ¶m); - void assertMirrorReady(const IMirrorAPI &mirror); - void assertMirrorContains(const IMirrorAPI &mirror, const string &pattern, uint32_t numEntries); + static void assertMirrorReady(const IMirrorAPI &mirror); + static void assertMirrorContains(const IMirrorAPI &mirror, const string &pattern, uint32_t numEntries); mbus::Message::UP newPutDocumentMessage(const string &documentId); public: Test(); - ~Test(); + ~Test() override; int Main() override; void testAND(); void testDocumentRouteSelector(); @@ -70,7 +70,7 @@ public: void get_document_messages_are_sent_to_the_route_handling_the_given_document_type(); void testExternSend(); void testExternMultipleSlobroks(); - void testLoadBalancer(); + static void testLoadBalancer(); void testLocalService(); void testLocalServiceCache(); void testProtocol(); @@ -101,8 +101,7 @@ int Test::Main() { TEST_INIT(_argv[0]); - _repo.reset(new DocumentTypeRepo(readDocumenttypesConfig( - TEST_PATH("../../../test/cfg/testdoctypes.cfg")))); + _repo = std::make_shared<DocumentTypeRepo>(readDocumenttypesConfig(TEST_PATH("../../../test/cfg/testdoctypes.cfg"))); _docType = _repo->getDocumentType("testdoc"); testProtocol(); TEST_FLUSH(); @@ -140,7 +139,7 @@ Test::Main() { void Test::testProtocol() { - mbus::IProtocol::SP protocol(new DocumentProtocol(_repo)); + auto protocol = std::make_shared<DocumentProtocol>(_repo); mbus::IRoutingPolicy::UP policy = protocol->createPolicy("AND", ""); ASSERT_TRUE(dynamic_cast<ANDPolicy*>(policy.get()) != nullptr); @@ -218,9 +217,9 @@ Test::requireThatExternPolicySelectsFromExternSlobrok() mbus::Slobrok slobrok; std::vector<mbus::TestServer*> servers; for (uint32_t i = 0; i < 10; ++i) { - mbus::TestServer *server = new mbus::TestServer( + auto *server = new mbus::TestServer( mbus::Identity(make_string("docproc/cluster.default/%d", i)), mbus::RoutingSpec(), slobrok, - mbus::IProtocol::SP(new DocumentProtocol(_repo))); + std::make_shared<DocumentProtocol>(_repo)); servers.push_back(server); server->net.registerSession("chain.default"); } @@ -231,12 +230,12 @@ Test::requireThatExternPolicySelectsFromExternSlobrok() ASSERT_TRUE(frame.select(leaf, 1)); lst.insert(leaf[0]->getRoute().toString()); - leaf[0]->handleReply(mbus::Reply::UP(new mbus::EmptyReply())); + leaf[0]->handleReply(std::make_unique<mbus::EmptyReply>()); ASSERT_TRUE(frame.getReceptor().getReply(TIMEOUT)); } EXPECT_EQUAL(servers.size(), lst.size()); - for (uint32_t i = 0; i < servers.size(); ++i) { - delete servers[i]; + for (auto & server : servers) { + delete server; } } @@ -247,7 +246,7 @@ Test::requireThatExternPolicyMergesOneReplyAsProtocol() frame.setMessage(newPutDocumentMessage("id:ns:testdoc::")); mbus::Slobrok slobrok; mbus::TestServer server(mbus::Identity("docproc/cluster.default/0"), mbus::RoutingSpec(), slobrok, - mbus::IProtocol::SP(new DocumentProtocol(_repo))); + std::make_shared<DocumentProtocol>(_repo)); server.net.registerSession("chain.default"); setupExternPolicy(frame, slobrok, "docproc/cluster.default/0/chain.default", 1); EXPECT_TRUE(frame.testMergeOneReply(server.net.getConnectionSpec() + "/chain.default")); @@ -256,23 +255,18 @@ Test::requireThatExternPolicyMergesOneReplyAsProtocol() mbus::Message::UP Test::newPutDocumentMessage(const string &documentId) { - Document::SP doc(new Document(*_docType, DocumentId(documentId))); - return make_unique<PutDocumentMessage>(doc); + return make_unique<PutDocumentMessage>(std::make_shared<Document>(*_docType, DocumentId(documentId))); } void -Test::setupExternPolicy(TestFrame &frame, mbus::Slobrok &slobrok, const string &pattern, - int32_t numEntries) +Test::setupExternPolicy(TestFrame &frame, mbus::Slobrok &slobrok, const string &pattern, int32_t numEntries) { string param = vespalib::make_string("tcp/localhost:%d;%s", slobrok.port(), pattern.c_str()); frame.setHop(mbus::HopSpec("test", vespalib::make_string("[Extern:%s]", param.c_str()))); mbus::MessageBus &mbus = frame.getMessageBus(); const mbus::HopBlueprint *hop = mbus.getRoutingTable(DocumentProtocol::NAME)->getHop("test"); - const mbus::PolicyDirective dir = static_cast<mbus::PolicyDirective&>(*hop->getDirective(0)); - ExternPolicy &policy = static_cast<ExternPolicy&>(*mbus.getRoutingPolicy( - DocumentProtocol::NAME, - dir.getName(), - dir.getParam())); + const mbus::PolicyDirective &dir = dynamic_cast<const mbus::PolicyDirective&>(*hop->getDirective(0)); + ExternPolicy &policy = dynamic_cast<ExternPolicy&>(*mbus.getRoutingPolicy(DocumentProtocol::NAME, dir.getName(), dir.getParam())); assertMirrorReady(*policy.getMirror()); if (numEntries >= 0) { assertMirrorContains(*policy.getMirror(), pattern, numEntries); @@ -316,7 +310,7 @@ Test::testExternSend() mbus::Slobrok slobrok; mbus::TestServer itr(mbus::Identity("itr"), mbus::RoutingSpec() .addTable(mbus::RoutingTableSpec(DocumentProtocol::NAME) - .addRoute(mbus::RouteSpec("default").addHop("dst")) + .addRoute(std::move(mbus::RouteSpec("default").addHop("dst"))) .addHop(mbus::HopSpec("dst", "dst/session"))), slobrok, std::make_shared<DocumentProtocol>(_repo)); mbus::Receptor ir; @@ -410,7 +404,7 @@ Test::testLocalService() ASSERT_TRUE(frame.select(leaf, 1)); lst.insert(leaf[0]->getRoute().toString()); - leaf[0]->handleReply(mbus::Reply::UP(new mbus::EmptyReply())); + leaf[0]->handleReply(std::make_unique<mbus::EmptyReply>()); ASSERT_TRUE(frame.getReceptor().getReply(TIMEOUT)); } EXPECT_EQUAL(10u, lst.size()); @@ -423,7 +417,7 @@ Test::testLocalService() ASSERT_TRUE(frame.select(leaf, 1)); lst.insert(leaf[0]->getRoute().toString()); - leaf[0]->handleReply(mbus::Reply::UP(new mbus::EmptyReply())); + leaf[0]->handleReply(std::make_unique<mbus::EmptyReply>()); ASSERT_TRUE(frame.getReceptor().getReply(TIMEOUT)); } EXPECT_EQUAL(1u, lst.size()); @@ -444,13 +438,13 @@ Test::testLocalServiceCache() TestFrame barFrame(fooFrame); mbus::HopSpec barHop("test", "docproc/cluster.default/[LocalService]/chain.bar"); - barFrame.setMessage(mbus::Message::UP(new GetDocumentMessage(DocumentId("id:ns:testdoc::bar")))); + barFrame.setMessage(std::make_unique<GetDocumentMessage>(DocumentId("id:ns:testdoc::bar"))); barFrame.setHop(barHop); fooFrame.getMessageBus().setupRouting( mbus::RoutingSpec().addTable(mbus::RoutingTableSpec(DocumentProtocol::NAME) - .addHop(fooHop) - .addHop(barHop))); + .addHop(std::move(fooHop)) + .addHop(std::move(barHop)))); fooFrame.getNetwork().registerSession("0/chain.foo"); fooFrame.getNetwork().registerSession("0/chain.bar"); @@ -464,8 +458,8 @@ Test::testLocalServiceCache() ASSERT_TRUE(barFrame.select(barSelected, 1)); EXPECT_EQUAL("docproc/cluster.default/0/chain.bar", barSelected[0]->getRoute().getHop(0).toString()); - barSelected[0]->handleReply(mbus::Reply::UP(new mbus::EmptyReply())); - fooSelected[0]->handleReply(mbus::Reply::UP(new mbus::EmptyReply())); + barSelected[0]->handleReply(std::make_unique<mbus::EmptyReply>()); + fooSelected[0]->handleReply(std::make_unique<mbus::EmptyReply>()); ASSERT_TRUE(barFrame.getReceptor().getReply(TIMEOUT)); ASSERT_TRUE(fooFrame.getReceptor().getReply(TIMEOUT)); @@ -515,19 +509,19 @@ Test::testRoundRobinCache() TestFrame fooFrame(_repo, "docproc/cluster.default"); mbus::HopSpec fooHop("foo", "[RoundRobin]"); fooHop.addRecipient("docproc/cluster.default/0/chain.foo"); - fooFrame.setMessage(mbus::Message::UP(new GetDocumentMessage(DocumentId("id:ns:testdoc::foo")))); + fooFrame.setMessage(std::make_unique<GetDocumentMessage>(DocumentId("id:ns:testdoc::foo"))); fooFrame.setHop(fooHop); TestFrame barFrame(fooFrame); mbus::HopSpec barHop("bar", "[RoundRobin]"); barHop.addRecipient("docproc/cluster.default/0/chain.bar"); - barFrame.setMessage(mbus::Message::UP(new GetDocumentMessage(DocumentId("id:ns:testdoc::bar")))); + barFrame.setMessage(std::make_unique<GetDocumentMessage>(DocumentId("id:ns:testdoc::bar"))); barFrame.setHop(barHop); fooFrame.getMessageBus().setupRouting( mbus::RoutingSpec().addTable(mbus::RoutingTableSpec(DocumentProtocol::NAME) - .addHop(fooHop) - .addHop(barHop))); + .addHop(std::move(fooHop)) + .addHop(std::move(barHop)))); fooFrame.getNetwork().registerSession("0/chain.foo"); fooFrame.getNetwork().registerSession("0/chain.bar"); @@ -541,8 +535,8 @@ Test::testRoundRobinCache() ASSERT_TRUE(barFrame.select(barSelected, 1)); EXPECT_EQUAL("docproc/cluster.default/0/chain.bar", barSelected[0]->getRoute().getHop(0).toString()); - barSelected[0]->handleReply(mbus::Reply::UP(new mbus::EmptyReply())); - fooSelected[0]->handleReply(mbus::Reply::UP(new mbus::EmptyReply())); + barSelected[0]->handleReply(std::make_unique<mbus::EmptyReply>()); + fooSelected[0]->handleReply(std::make_unique<mbus::EmptyReply>()); ASSERT_TRUE(barFrame.getReceptor().getReply(TIMEOUT)); ASSERT_TRUE(fooFrame.getReceptor().getReply(TIMEOUT)); @@ -568,16 +562,16 @@ Test::multipleGetRepliesAreMergedToFoundDocument() for (uint32_t i = 0, len = selected.size(); i < len; ++i) { Document::SP doc; if (i == 0) { - doc.reset(new Document(*_docType, DocumentId("id:ns:testdoc::yarn"))); + doc = std::make_shared<Document>(*_docType, DocumentId("id:ns:testdoc::yarn")); doc->setLastModified(123456ULL); } - mbus::Reply::UP reply(new GetDocumentReply(std::move(doc))); + auto reply = std::make_unique<GetDocumentReply>(std::move(doc)); selected[i]->handleReply(std::move(reply)); } mbus::Reply::UP reply = frame.getReceptor().getReply(TIMEOUT); EXPECT_TRUE(reply); EXPECT_EQUAL(static_cast<uint32_t>(DocumentProtocol::REPLY_GETDOCUMENT), reply->getType()); - EXPECT_EQUAL(123456ULL, static_cast<GetDocumentReply&>(*reply).getLastModified()); + EXPECT_EQUAL(123456ULL, dynamic_cast<GetDocumentReply&>(*reply).getLastModified()); } void @@ -621,7 +615,7 @@ Test::testDocumentRouteSelector() frame.setMessage(std::move(put)); EXPECT_TRUE(frame.testSelect( StringList().add("foo"))); - frame.setMessage(mbus::Message::UP(new RemoveDocumentMessage(DocumentId("id:ns:testdoc::")))); + frame.setMessage(std::make_unique<RemoveDocumentMessage>(DocumentId("id:ns:testdoc::"))); EXPECT_TRUE(frame.testSelect(StringList().add("foo"))); frame.setMessage(make_unique<UpdateDocumentMessage>( @@ -674,7 +668,7 @@ createDocumentRouteSelectorConfigWithTwoRoutes() } std::unique_ptr<TestFrame> -createFrameWithTwoRoutes(std::shared_ptr<const DocumentTypeRepo> repo) +createFrameWithTwoRoutes(const std::shared_ptr<const DocumentTypeRepo> & repo) { auto result = std::make_unique<TestFrame>(repo); result->setHop(mbus::HopSpec("test", createDocumentRouteSelectorConfigWithTwoRoutes()) @@ -683,13 +677,13 @@ createFrameWithTwoRoutes(std::shared_ptr<const DocumentTypeRepo> repo) } std::unique_ptr<RemoveDocumentMessage> -makeRemove(vespalib::string docId) +makeRemove(vespalib::stringref docId) { return std::make_unique<RemoveDocumentMessage>(DocumentId(docId)); } std::unique_ptr<GetDocumentMessage> -makeGet(vespalib::string docId) +makeGet(vespalib::stringref docId) { return std::make_unique<GetDocumentMessage>(DocumentId(docId)); } @@ -742,9 +736,9 @@ void Test::testLoadBalancer() { LoadBalancer lb("foo", ""); IMirrorAPI::SpecList entries; - entries.push_back(IMirrorAPI::Spec("foo/0/default", "tcp/bar:1")); - entries.push_back(IMirrorAPI::Spec("foo/1/default", "tcp/bar:2")); - entries.push_back(IMirrorAPI::Spec("foo/2/default", "tcp/bar:3")); + entries.emplace_back("foo/0/default", "tcp/bar:1"); + entries.emplace_back("foo/1/default", "tcp/bar:2"); + entries.emplace_back("foo/2/default", "tcp/bar:3"); for (int i = 0; i < 99; i++) { std::pair<string, int> recipient = lb.getRecipient(entries); @@ -796,10 +790,10 @@ Test::requireThatContentPolicyIsRandomWithoutState() mbus::Slobrok slobrok; std::vector<mbus::TestServer*> servers; for (uint32_t i = 0; i < 5; ++i) { - mbus::TestServer *srv = new mbus::TestServer( + auto *srv = new mbus::TestServer( mbus::Identity(vespalib::make_string("storage/cluster.mycluster/distributor/%d", i)), mbus::RoutingSpec(), slobrok, - mbus::IProtocol::SP(new DocumentProtocol(_repo))); + std::make_shared<DocumentProtocol>(_repo)); servers.push_back(srv); srv->net.registerSession("default"); } @@ -816,11 +810,11 @@ Test::requireThatContentPolicyIsRandomWithoutState() std::vector<mbus::RoutingNode*> leaf; ASSERT_TRUE(frame.select(leaf, 1)); lst.insert(leaf[0]->getRoute().toString()); - leaf[0]->handleReply(mbus::Reply::UP(new mbus::EmptyReply())); + leaf[0]->handleReply(std::make_unique<mbus::EmptyReply>()); } EXPECT_EQUAL(servers.size(), lst.size()); - for (uint32_t i = 0; i < servers.size(); ++i) { - delete servers[i]; + for (auto & server : servers) { + delete server; } } @@ -831,9 +825,8 @@ Test::setupContentPolicy(TestFrame &frame, const string ¶m, frame.setHop(mbus::HopSpec("test", vespalib::make_string("[Content:%s]", param.c_str()))); mbus::MessageBus &mbus = frame.getMessageBus(); const mbus::HopBlueprint *hop = mbus.getRoutingTable(DocumentProtocol::NAME)->getHop("test"); - const mbus::PolicyDirective dir = static_cast<mbus::PolicyDirective&>(*hop->getDirective(0)); - ContentPolicy &policy = static_cast<ContentPolicy&>(*mbus.getRoutingPolicy(DocumentProtocol::NAME, - dir.getName(), dir.getParam())); + const mbus::PolicyDirective & dir = dynamic_cast<const mbus::PolicyDirective&>(*hop->getDirective(0)); + ContentPolicy &policy = dynamic_cast<ContentPolicy&>(*mbus.getRoutingPolicy(DocumentProtocol::NAME, dir.getName(), dir.getParam())); policy.initSynchronous(); assertMirrorReady(*policy.getMirror()); if (numEntries >= 0) { @@ -851,7 +844,7 @@ Test::requireThatContentPolicyIsTargetedWithState() mbus::Slobrok slobrok; std::vector<mbus::TestServer*> servers; for (uint32_t i = 0; i < 5; ++i) { - mbus::TestServer *srv = new mbus::TestServer( + auto *srv = new mbus::TestServer( mbus::Identity(vespalib::make_string("storage/cluster.mycluster/distributor/%d", i)), mbus::RoutingSpec(), slobrok, make_shared<DocumentProtocol>(_repo)); @@ -861,14 +854,12 @@ Test::requireThatContentPolicyIsTargetedWithState() string param = vespalib::make_string( "cluster=mycluster;slobroks=tcp/localhost:%d;clusterconfigid=%s;syncinit", slobrok.port(), getDefaultDistributionConfig(2, 5).c_str()); - ContentPolicy &policy = setupContentPolicy( - frame, param, - "storage/cluster.mycluster/distributor/*/default", 5); + ContentPolicy &policy = setupContentPolicy(frame, param, "storage/cluster.mycluster/distributor/*/default", 5); ASSERT_TRUE(policy.getSystemState() == nullptr); { std::vector<mbus::RoutingNode*> leaf; ASSERT_TRUE(frame.select(leaf, 1)); - leaf[0]->handleReply(mbus::Reply::UP(new WrongDistributionReply("distributor:5 storage:5"))); + leaf[0]->handleReply(std::make_unique<WrongDistributionReply>("distributor:5 storage:5")); ASSERT_TRUE(policy.getSystemState() != nullptr); EXPECT_EQUAL(policy.getSystemState()->toString(), "distributor:5 storage:5"); } @@ -877,11 +868,11 @@ Test::requireThatContentPolicyIsTargetedWithState() std::vector<mbus::RoutingNode*> leaf; ASSERT_TRUE(frame.select(leaf, 1)); lst.insert(leaf[0]->getRoute().toString()); - leaf[0]->handleReply(mbus::Reply::UP(new mbus::EmptyReply())); + leaf[0]->handleReply(std::make_unique<mbus::EmptyReply>()); } EXPECT_EQUAL(1u, lst.size()); - for (uint32_t i = 0; i < servers.size(); ++i) { - delete servers[i]; + for (auto & server : servers) { + delete server; } } @@ -907,7 +898,7 @@ Test::requireThatContentPolicyCombinesSystemAndSlobrokState() { std::vector<mbus::RoutingNode*> leaf; ASSERT_TRUE(frame.select(leaf, 1)); - leaf[0]->handleReply(mbus::Reply::UP(new WrongDistributionReply("distributor:99 storage:99"))); + leaf[0]->handleReply(std::make_unique<WrongDistributionReply>("distributor:99 storage:99")); ASSERT_TRUE(policy.getSystemState() != nullptr); EXPECT_EQUAL(policy.getSystemState()->toString(), "distributor:99 storage:99"); } @@ -933,7 +924,7 @@ Test::testSubsetService() std::vector<mbus::RoutingNode*> leaf; ASSERT_TRUE(frame.select(leaf, 1)); lst.insert(leaf[0]->getRoute().toString()); - leaf[0]->handleReply(mbus::Reply::UP(new mbus::EmptyReply())); + leaf[0]->handleReply(std::make_unique<mbus::EmptyReply>()); ASSERT_TRUE(frame.getReceptor().getReply(TIMEOUT)); } ASSERT_TRUE(lst.size() > 1); // must have requeried @@ -952,7 +943,7 @@ Test::testSubsetService() } prev = next; - leaf[0]->handleReply(mbus::Reply::UP(new mbus::EmptyReply())); + leaf[0]->handleReply(std::make_unique<mbus::EmptyReply>()); ASSERT_TRUE(frame.getReceptor().getReply(TIMEOUT)); } @@ -967,7 +958,7 @@ Test::testSubsetService() frame.getNetwork().unregisterSession(route.substr(frame.getIdentity().length() + 1)); ASSERT_TRUE(frame.waitSlobrok("docproc/cluster.default/*/chain.default", 10 - i)); - mbus::Reply::UP reply(new mbus::EmptyReply()); + auto reply = std::make_unique<mbus::EmptyReply>(); reply->addError(mbus::Error(mbus::ErrorCode::NO_ADDRESS_FOR_SERVICE, route)); leaf[0]->handleReply(std::move(reply)); ASSERT_TRUE(frame.getReceptor().getReply(TIMEOUT)); @@ -984,18 +975,18 @@ Test::testSubsetServiceCache() { TestFrame fooFrame(_repo, "docproc/cluster.default"); mbus::HopSpec fooHop("foo", "docproc/cluster.default/[SubsetService:2]/chain.foo"); - fooFrame.setMessage(mbus::Message::UP(new GetDocumentMessage(DocumentId("id:ns:testdoc::foo")))); + fooFrame.setMessage(std::make_unique<GetDocumentMessage>(DocumentId("id:ns:testdoc::foo"))); fooFrame.setHop(fooHop); TestFrame barFrame(fooFrame); mbus::HopSpec barHop("bar", "docproc/cluster.default/[SubsetService:2]/chain.bar"); - barFrame.setMessage(mbus::Message::UP(new GetDocumentMessage(DocumentId("id:ns:testdoc::bar")))); + barFrame.setMessage(std::make_unique<GetDocumentMessage>(DocumentId("id:ns:testdoc::bar"))); barFrame.setHop(barHop); fooFrame.getMessageBus().setupRouting( mbus::RoutingSpec().addTable(mbus::RoutingTableSpec(DocumentProtocol::NAME) - .addHop(fooHop) - .addHop(barHop))); + .addHop(std::move(fooHop)) + .addHop(std::move(barHop)))); fooFrame.getNetwork().registerSession("0/chain.foo"); fooFrame.getNetwork().registerSession("0/chain.bar"); @@ -1009,8 +1000,8 @@ Test::testSubsetServiceCache() ASSERT_TRUE(barFrame.select(barSelected, 1)); EXPECT_EQUAL("docproc/cluster.default/0/chain.bar", barSelected[0]->getRoute().getHop(0).toString()); - barSelected[0]->handleReply(mbus::Reply::UP(new mbus::EmptyReply())); - fooSelected[0]->handleReply(mbus::Reply::UP(new mbus::EmptyReply())); + barSelected[0]->handleReply(std::make_unique<mbus::EmptyReply>()); + fooSelected[0]->handleReply(std::make_unique<mbus::EmptyReply>()); ASSERT_TRUE(barFrame.getReceptor().getReply(TIMEOUT)); ASSERT_TRUE(fooFrame.getReceptor().getReply(TIMEOUT)); @@ -1024,7 +1015,7 @@ Test::trySelect(TestFrame &frame, uint32_t numSelects, const std::vector<string> if (!expected.empty()) { frame.select(leaf, 1); lst.insert(leaf[0]->getRoute().toString()); - leaf[0]->handleReply(mbus::Reply::UP(new mbus::EmptyReply())); + leaf[0]->handleReply(std::make_unique<mbus::EmptyReply>()); } else { frame.select(leaf, 0); } @@ -1037,7 +1028,7 @@ Test::trySelect(TestFrame &frame, uint32_t numSelects, const std::vector<string> LOG(error, "Expected %d recipients, got %d.", (uint32_t)expected.size(), (uint32_t)lst.size()); return false; } - std::set<string>::iterator it = lst.begin(); + auto it = lst.begin(); for (uint32_t i = 0; i < expected.size(); ++i, ++it) { if (*it != expected[i]) { LOG(error, "Expected '%s', got '%s'.", expected[i].c_str(), it->c_str()); diff --git a/documentapi/src/tests/policies/testframe.cpp b/documentapi/src/tests/policies/testframe.cpp index cb33ddd649b..973297a50d1 100644 --- a/documentapi/src/tests/policies/testframe.cpp +++ b/documentapi/src/tests/policies/testframe.cpp @@ -22,7 +22,7 @@ private: string _address; public: - MyServiceAddress(const string &address) : _address(address) {} + explicit MyServiceAddress(const string &address) : _address(address) {} const string &getAddress() { return _address; } }; @@ -32,7 +32,7 @@ private: std::vector<mbus::RoutingNode*> _nodes; public: - MyNetwork(const mbus::RPCNetworkParams ¶ms) : + explicit MyNetwork(const mbus::RPCNetworkParams ¶ms) : mbus::RPCNetwork(params), _nodes() { } @@ -86,7 +86,7 @@ void TestFrame::setHop(const mbus::HopSpec &hop) { _hop = hop; - _mbus->setupRouting(mbus::RoutingSpec().addTable(mbus::RoutingTableSpec(DocumentProtocol::NAME).addHop(_hop))); + _mbus->setupRouting(mbus::RoutingSpec().addTable(mbus::RoutingTableSpec(DocumentProtocol::NAME).addHop(mbus::HopSpec(_hop)))); } bool @@ -111,8 +111,8 @@ TestFrame::testSelect(const std::vector<string> &expected) std::vector<mbus::RoutingNode*> selected; if (!select(selected, expected.size())) { LOG(error, "Failed to select recipients."); - for (size_t i = 0; i < selected.size(); ++i) { - LOG(error, "Selected: %s", selected[i]->getRoute().toString().c_str()); + for (auto & i : selected) { + LOG(error, "Selected: %s", i->getRoute().toString().c_str()); } return false; } @@ -154,7 +154,7 @@ TestFrame::testMerge(const ReplyMap &replies, for (mbus::RoutingNode* node : selected) { string route = node->getRoute().toString(); - ReplyMap::const_iterator mip = replies.find(route); + auto mip = replies.find(route); if (mip == replies.end()) { LOG(error, "Recipient '%s' not expected.", route.c_str()); return false; diff --git a/messagebus/src/tests/advancedrouting/advancedrouting.cpp b/messagebus/src/tests/advancedrouting/advancedrouting.cpp index fbe6c8f77bd..1711122df8a 100644 --- a/messagebus/src/tests/advancedrouting/advancedrouting.cpp +++ b/messagebus/src/tests/advancedrouting/advancedrouting.cpp @@ -123,7 +123,7 @@ Test::testAdvanced(TestData &data) data._srcServer.mb.setupRouting(RoutingSpec().addTable(RoutingTableSpec(SimpleProtocol::NAME) .addHop(HopSpec("bar", "dst/bar")) .addHop(HopSpec("baz", "dst/baz")) - .addRoute(RouteSpec("baz").addHop("baz")))); + .addRoute(std::move(RouteSpec("baz").addHop("baz"))))); string route = vespalib::make_string("[Custom:%s,bar,route:baz,dst/cox,?dst/unknown]", data._fooSession->getConnectionSpec().c_str()); EXPECT_TRUE(data._srcSession->send(createMessage("msg"), Route::parse(route)).isAccepted()); diff --git a/messagebus/src/tests/context/context.cpp b/messagebus/src/tests/context/context.cpp index 85b92cc6fa0..e91946aab05 100644 --- a/messagebus/src/tests/context/context.cpp +++ b/messagebus/src/tests/context/context.cpp @@ -18,7 +18,7 @@ struct Handler : public IMessageHandler { DestinationSession::UP session; - Handler(MessageBus &mb) : session() { + explicit Handler(MessageBus &mb) : session() { session = mb.createDestinationSession("session", true, *this); } ~Handler() override { @@ -33,7 +33,7 @@ RoutingSpec getRouting() { return RoutingSpec() .addTable(RoutingTableSpec("Simple") .addHop(HopSpec("test", "test/session")) - .addRoute(RouteSpec("test").addHop("test"))); + .addRoute(std::move(RouteSpec("test").addHop("test")))); } TEST_SETUP(Test); diff --git a/messagebus/src/tests/error/error.cpp b/messagebus/src/tests/error/error.cpp index da7966621fa..d97440f12e0 100644 --- a/messagebus/src/tests/error/error.cpp +++ b/messagebus/src/tests/error/error.cpp @@ -4,14 +4,11 @@ #include <vespa/messagebus/emptyreply.h> #include <vespa/messagebus/errorcode.h> #include <vespa/messagebus/intermediatesession.h> -#include <vespa/messagebus/messagebus.h> #include <vespa/messagebus/routing/routingspec.h> #include <vespa/messagebus/sourcesession.h> #include <vespa/messagebus/sourcesessionparams.h> #include <vespa/messagebus/testlib/receptor.h> #include <vespa/messagebus/testlib/simplemessage.h> -#include <vespa/messagebus/testlib/simplereply.h> -#include <vespa/messagebus/testlib/simpleprotocol.h> #include <vespa/messagebus/testlib/slobrok.h> #include <vespa/messagebus/testlib/testserver.h> #include <vespa/vespalib/testkit/testapp.h> @@ -23,7 +20,7 @@ RoutingSpec getRouting() { .addTable(RoutingTableSpec("Simple") .addHop(HopSpec("pxy", "test/pxy/session")) .addHop(HopSpec("dst", "test/dst/session")) - .addRoute(RouteSpec("test").addHop("pxy").addHop("dst"))); + .addRoute(std::move(RouteSpec("test").addHop("pxy").addHop("dst")))); } TEST("error_test") { diff --git a/messagebus/src/tests/messagebus/messagebus.cpp b/messagebus/src/tests/messagebus/messagebus.cpp index 1abf5ae8ab2..ba466c4210e 100644 --- a/messagebus/src/tests/messagebus/messagebus.cpp +++ b/messagebus/src/tests/messagebus/messagebus.cpp @@ -30,14 +30,14 @@ struct Base { return RoutingSpec() .addTable(RoutingTableSpec("Simple") .addHop(HopSpec("DocProc", "docproc/*/session")) - .addHop(HopSpec("Search", "search/[All]/[Hash]/session") + .addHop(std::move(HopSpec("Search", "search/[All]/[Hash]/session") .addRecipient("search/r.0/c.0/session") .addRecipient("search/r.0/c.1/session") .addRecipient("search/r.1/c.0/session") - .addRecipient("search/r.1/c.1/session")) - .addRoute(RouteSpec("Index").addHop("DocProc").addHop("Search")) - .addRoute(RouteSpec("DocProc").addHop("DocProc")) - .addRoute(RouteSpec("Search").addHop("Search"))); + .addRecipient("search/r.1/c.1/session"))) + .addRoute(std::move(RouteSpec("Index").addHop("DocProc").addHop("Search"))) + .addRoute(std::move(RouteSpec("DocProc").addHop("DocProc"))) + .addRoute(std::move(RouteSpec("Search").addHop("Search")))); } bool waitQueueSize(uint32_t size) { for (uint32_t i = 0; i < 1000; ++i) { @@ -71,8 +71,11 @@ struct Server : public Base { { // empty } + ~Server() override; }; +Server::~Server() = default; + struct DocProc : public Server { using UP = std::unique_ptr<DocProc>; IntermediateSession::UP session; diff --git a/messagebus/src/tests/messageordering/messageordering.cpp b/messagebus/src/tests/messageordering/messageordering.cpp index e82e58d1730..d83ef59be28 100644 --- a/messagebus/src/tests/messageordering/messageordering.cpp +++ b/messagebus/src/tests/messageordering/messageordering.cpp @@ -23,7 +23,7 @@ getRouting() return RoutingSpec() .addTable(RoutingTableSpec("Simple") .addHop(HopSpec("dst", "test/dst/session")) - .addRoute(RouteSpec("test").addHop("dst"))); + .addRoute(std::move(RouteSpec("test").addHop("dst")))); } class MultiReceptor : public IMessageHandler @@ -113,7 +113,7 @@ VerifyReplyReceptor::handleReply(Reply::UP reply) LOG(warning, "%s", ss.str().c_str()); } else { vespalib::string expected(vespalib::make_string("%d", _replyCount)); - auto & simpleReply(static_cast<SimpleReply&>(*reply)); + auto & simpleReply(dynamic_cast<SimpleReply&>(*reply)); if (simpleReply.getValue() != expected) { std::stringstream ss; ss << "Received out-of-sequence reply! Expected " diff --git a/messagebus/src/tests/routing/routing.cpp b/messagebus/src/tests/routing/routing.cpp index acdcf55fe52..2c4056b868d 100644 --- a/messagebus/src/tests/routing/routing.cpp +++ b/messagebus/src/tests/routing/routing.cpp @@ -804,7 +804,7 @@ void Test::testRecognizeRouteDirective(TestData &data) { data._srcServer.mb.setupRouting(RoutingSpec().addTable(RoutingTableSpec(SimpleProtocol::NAME) - .addRoute(RouteSpec("dst").addHop("dst/session")) + .addRoute(std::move(RouteSpec("dst").addHop("dst/session"))) .addHop(HopSpec("dir", "route:dst")))); EXPECT_TRUE(data._srcSession->send(createMessage("msg"), Route::parse("dir")).isAccepted()); Message::UP msg = data._dstHandler.getMessage(RECEPTOR_TIMEOUT); @@ -819,7 +819,7 @@ void Test::testRecognizeRouteName(TestData &data) { data._srcServer.mb.setupRouting(RoutingSpec().addTable(RoutingTableSpec(SimpleProtocol::NAME) - .addRoute(RouteSpec("dst").addHop("dst/session")))); + .addRoute(std::move(RouteSpec("dst").addHop("dst/session"))))); EXPECT_TRUE(data._srcSession->send(createMessage("msg"), Route::parse("dst")).isAccepted()); Message::UP msg = data._dstHandler.getMessage(RECEPTOR_TIMEOUT); ASSERT_TRUE(msg); @@ -846,7 +846,7 @@ void Test::testRouteResolutionOverflow(TestData &data) { data._srcServer.mb.setupRouting(RoutingSpec().addTable(RoutingTableSpec(SimpleProtocol::NAME) - .addRoute(RouteSpec("foo").addHop("route:foo")))); + .addRoute(std::move(RouteSpec("foo").addHop("route:foo"))))); EXPECT_TRUE(data._srcSession->send(createMessage("msg"), "foo").isAccepted()); Reply::UP reply = data._srcHandler.getReply(RECEPTOR_TIMEOUT); ASSERT_TRUE(reply); @@ -858,7 +858,7 @@ void Test::testInsertRoute(TestData &data) { data._srcServer.mb.setupRouting(RoutingSpec().addTable(RoutingTableSpec(SimpleProtocol::NAME) - .addRoute(RouteSpec("foo").addHop("dst/session").addHop("bar")))); + .addRoute(std::move(RouteSpec("foo").addHop("dst/session").addHop("bar"))))); EXPECT_TRUE(data._srcSession->send(createMessage("msg"), Route::parse("route:foo baz")).isAccepted()); Message::UP msg = data._dstHandler.getMessage(RECEPTOR_TIMEOUT); ASSERT_TRUE(msg); @@ -1302,7 +1302,7 @@ void Test::testHopBlueprintIgnoresReply(TestData &data) { data._srcServer.mb.setupRouting(RoutingSpec().addTable(RoutingTableSpec(SimpleProtocol::NAME) - .addHop(HopSpec("foo", "dst/session").setIgnoreResult(true)))); + .addHop(std::move(HopSpec("foo", "dst/session").setIgnoreResult(true))))); EXPECT_TRUE(data._srcSession->send(createMessage("msg"), Route::parse("foo")).isAccepted()); Message::UP msg = data._dstHandler.getMessage(RECEPTOR_TIMEOUT); ASSERT_TRUE(msg); @@ -1419,7 +1419,7 @@ Test::requireThatIgnoreFlagPersistsThroughHopLookup(TestData &data) void Test::requireThatIgnoreFlagPersistsThroughRouteLookup(TestData &data) { - setupRouting(data, RoutingTableSpec(SimpleProtocol::NAME).addRoute(RouteSpec("foo").addHop("dst/unknown"))); + setupRouting(data, RoutingTableSpec(SimpleProtocol::NAME).addRoute(std::move(RouteSpec("foo").addHop("dst/unknown")))); ASSERT_TRUE(testSend(data, "?foo")); ASSERT_TRUE(testTrace(data, StringList().add("Ignoring errors in reply."))); } diff --git a/messagebus/src/tests/routingcontext/routingcontext.cpp b/messagebus/src/tests/routingcontext/routingcontext.cpp index aba50156412..c21725a482e 100644 --- a/messagebus/src/tests/routingcontext/routingcontext.cpp +++ b/messagebus/src/tests/routingcontext/routingcontext.cpp @@ -59,7 +59,7 @@ private: CustomPolicyFactory &_factory; public: - CustomPolicy(CustomPolicyFactory &factory); + explicit CustomPolicy(CustomPolicyFactory &factory); void select(RoutingContext &ctx) override; void merge(RoutingContext &ctx) override; }; @@ -73,21 +73,19 @@ CustomPolicy::CustomPolicy(CustomPolicyFactory &factory) : void CustomPolicy::select(RoutingContext &ctx) { - Reply::UP reply(new EmptyReply()); + auto reply = std::make_unique<EmptyReply>(); reply->getTrace().setLevel(9); const std::vector<Route> &all = ctx.getAllRecipients(); if (_factory._expectedAll.size() == all.size()) { ctx.trace(1, make_string("Got %d expected recipients.", (uint32_t)all.size())); - for (std::vector<Route>::const_iterator it = all.begin(); - it != all.end(); ++it) - { - if (find(_factory._expectedAll.begin(), _factory._expectedAll.end(), it->toString()) != _factory._expectedAll.end()) { - ctx.trace(1, make_string("Got expected recipient '%s'.", it->toString().c_str())); + for (const auto & it : all) { + if (find(_factory._expectedAll.begin(), _factory._expectedAll.end(), it.toString()) != _factory._expectedAll.end()) { + ctx.trace(1, make_string("Got expected recipient '%s'.", it.toString().c_str())); } else { reply->addError(Error(ErrorCode::APP_FATAL_ERROR, make_string("Matched recipient '%s' not expected.", - it->toString().c_str()))); + it.toString().c_str()))); } } } else { @@ -115,15 +113,12 @@ CustomPolicy::select(RoutingContext &ctx) ctx.getMatchedRecipients(matched); if (_factory._expectedMatched.size() == matched.size()) { ctx.trace(1, make_string("Got %d expected recipients.", (uint32_t)matched.size())); - for (std::vector<Route>::iterator it = matched.begin(); - it != matched.end(); ++it) - { - if (find(_factory._expectedMatched.begin(), _factory._expectedMatched.end(), it->toString()) != _factory._expectedMatched.end()) { - ctx.trace(1, make_string("Got matched recipient '%s'.", it->toString().c_str())); + for (auto & it : matched) { + if (find(_factory._expectedMatched.begin(), _factory._expectedMatched.end(), it.toString()) != _factory._expectedMatched.end()) { + ctx.trace(1, make_string("Got matched recipient '%s'.", it.toString().c_str())); } else { reply->addError(Error(ErrorCode::APP_FATAL_ERROR, - make_string("Matched recipient '%s' not expected.", - it->toString().c_str()))); + make_string("Matched recipient '%s' not expected.", it.toString().c_str()))); } } } else { @@ -134,10 +129,8 @@ CustomPolicy::select(RoutingContext &ctx) } if (!reply->hasErrors() && _factory._forward) { - for (std::vector<Route>::iterator it = matched.begin(); - it != matched.end(); ++it) - { - ctx.addChild(*it); + for (auto & it : matched) { + ctx.addChild(it); } } else { ctx.setReply(std::move(reply)); @@ -258,7 +251,7 @@ TestData::start() Message::UP Test::createMessage(const string &msg) { - Message::UP ret(new SimpleMessage(msg)); + auto ret = std::make_unique<SimpleMessage>(msg); ret->getTrace().setLevel(9); return ret; } @@ -289,22 +282,22 @@ void Test::testSingleDirective(TestData &data) { IProtocol::SP protocol(new SimpleProtocol()); - SimpleProtocol &simple = static_cast<SimpleProtocol&>(*protocol); + auto &simple = dynamic_cast<SimpleProtocol&>(*protocol); simple.addPolicyFactory("Custom", SimpleProtocol::IPolicyFactory::SP(new CustomPolicyFactory( false, StringList().add("foo").add("bar").add("baz/cox"), StringList().add("foo").add("bar")))); data._srcServer.mb.putProtocol(protocol); data._srcServer.mb.setupRouting(RoutingSpec().addTable(RoutingTableSpec(SimpleProtocol::NAME) - .addRoute(RouteSpec("myroute").addHop("myhop")) - .addHop(HopSpec("myhop", "[Custom]") + .addRoute(std::move(RouteSpec("myroute").addHop("myhop"))) + .addHop(std::move(HopSpec("myhop", "[Custom]") .addRecipient("foo") .addRecipient("bar") - .addRecipient("baz/cox")))); + .addRecipient("baz/cox"))))); for (uint32_t i = 0; i < 2; ++i) { EXPECT_TRUE(data._srcSession->send(createMessage("msg"), "myroute").isAccepted()); Reply::UP reply = data._srcHandler.getReply(); - ASSERT_TRUE(reply.get() != NULL); + ASSERT_TRUE(reply); printf("%s", reply->getTrace().toString().c_str()); EXPECT_TRUE(!reply->hasErrors()); } @@ -314,24 +307,24 @@ void Test::testMoreDirectives(TestData &data) { IProtocol::SP protocol(new SimpleProtocol()); - SimpleProtocol &simple = static_cast<SimpleProtocol&>(*protocol); + auto &simple = dynamic_cast<SimpleProtocol&>(*protocol); simple.addPolicyFactory("Custom", SimpleProtocol::IPolicyFactory::SP(new CustomPolicyFactory( false, StringList().add("foo").add("foo/bar").add("foo/bar0/baz").add("foo/bar1/baz").add("foo/bar/baz/cox"), StringList().add("foo/bar0/baz").add("foo/bar1/baz")))); data._srcServer.mb.putProtocol(protocol); data._srcServer.mb.setupRouting(RoutingSpec().addTable(RoutingTableSpec(SimpleProtocol::NAME) - .addRoute(RouteSpec("myroute").addHop("myhop")) - .addHop(HopSpec("myhop", "foo/[Custom]/baz") + .addRoute(std::move(RouteSpec("myroute").addHop("myhop"))) + .addHop(std::move(HopSpec("myhop", "foo/[Custom]/baz") .addRecipient("foo") .addRecipient("foo/bar") .addRecipient("foo/bar0/baz") .addRecipient("foo/bar1/baz") - .addRecipient("foo/bar/baz/cox")))); + .addRecipient("foo/bar/baz/cox"))))); for (uint32_t i = 0; i < 2; ++i) { EXPECT_TRUE(data._srcSession->send(createMessage("msg"), "myroute").isAccepted()); Reply::UP reply = data._srcHandler.getReply(); - ASSERT_TRUE(reply.get() != NULL); + ASSERT_TRUE(reply); printf("%s", reply->getTrace().toString().c_str()); EXPECT_TRUE(!reply->hasErrors()); } @@ -341,7 +334,7 @@ void Test::testRecipientsRemain(TestData &data) { IProtocol::SP protocol(new SimpleProtocol()); - SimpleProtocol &simple = static_cast<SimpleProtocol&>(*protocol); + auto &simple = dynamic_cast<SimpleProtocol&>(*protocol); simple.addPolicyFactory("First", SimpleProtocol::IPolicyFactory::SP(new CustomPolicyFactory( true, StringList().add("foo/bar"), @@ -352,13 +345,13 @@ Test::testRecipientsRemain(TestData &data) StringList().add("foo/bar")))); data._srcServer.mb.putProtocol(protocol); data._srcServer.mb.setupRouting(RoutingSpec().addTable(RoutingTableSpec(SimpleProtocol::NAME) - .addRoute(RouteSpec("myroute").addHop("myhop")) - .addHop(HopSpec("myhop", "[First]/[Second]") - .addRecipient("foo/bar")))); + .addRoute(std::move(RouteSpec("myroute").addHop("myhop"))) + .addHop(std::move(HopSpec("myhop", "[First]/[Second]") + .addRecipient("foo/bar"))))); for (uint32_t i = 0; i < 2; ++i) { EXPECT_TRUE(data._srcSession->send(createMessage("msg"), "myroute").isAccepted()); Reply::UP reply = data._srcHandler.getReply(); - ASSERT_TRUE(reply.get() != NULL); + ASSERT_TRUE(reply); printf("%s", reply->getTrace().toString().c_str()); EXPECT_TRUE(!reply->hasErrors()); } @@ -367,25 +360,22 @@ Test::testRecipientsRemain(TestData &data) void Test::testConstRoute(TestData &data) { - IProtocol::SP protocol(new SimpleProtocol()); - SimpleProtocol &simple = static_cast<SimpleProtocol&>(*protocol); + auto protocol = std::make_shared<SimpleProtocol>(); + auto &simple = dynamic_cast<SimpleProtocol&>(*protocol); simple.addPolicyFactory("DocumentRouteSelector", - SimpleProtocol::IPolicyFactory::SP(new CustomPolicyFactory( - true, - StringList().add("dst"), - StringList().add("dst")))); + std::make_unique<CustomPolicyFactory>(true, StringList().add("dst"), StringList().add("dst"))); data._srcServer.mb.putProtocol(protocol); data._srcServer.mb.setupRouting(RoutingSpec().addTable(RoutingTableSpec(SimpleProtocol::NAME) - .addRoute(RouteSpec("default").addHop("indexing")) - .addHop(HopSpec("indexing", "[DocumentRouteSelector]").addRecipient("dst")) + .addRoute(std::move(RouteSpec("default").addHop("indexing"))) + .addHop(std::move(HopSpec("indexing", "[DocumentRouteSelector]").addRecipient("dst"))) .addHop(HopSpec("dst", "dst/session")))); for (uint32_t i = 0; i < 2; ++i) { EXPECT_TRUE(data._srcSession->send(createMessage("msg"), Route::parse("route:default")).isAccepted()); Message::UP msg = data._dstHandler.getMessage(TIMEOUT); - ASSERT_TRUE(msg.get() != NULL); + ASSERT_TRUE(msg); data._dstSession->acknowledge(std::move(msg)); Reply::UP reply = data._srcHandler.getReply(); - ASSERT_TRUE(reply.get() != NULL); + ASSERT_TRUE(reply); printf("%s", reply->getTrace().toString().c_str()); EXPECT_TRUE(!reply->hasErrors()); } diff --git a/messagebus/src/tests/routingspec/routingspec.cpp b/messagebus/src/tests/routingspec/routingspec.cpp index c3e6754ee7d..e7f4dec4f48 100644 --- a/messagebus/src/tests/routingspec/routingspec.cpp +++ b/messagebus/src/tests/routingspec/routingspec.cpp @@ -94,31 +94,31 @@ Test::testConstructors() { RoutingSpec spec = RoutingSpec() .addTable(RoutingTableSpec("foo") - .addHop(HopSpec("foo-h1", "foo-h1-sel") + .addHop(std::move(HopSpec("foo-h1", "foo-h1-sel") .addRecipient("foo-h1-r1") - .addRecipient("foo-h1-r2")) - .addHop(HopSpec("foo-h2", "foo-h2-sel") + .addRecipient("foo-h1-r2"))) + .addHop(std::move(HopSpec("foo-h2", "foo-h2-sel") .addRecipient("foo-h2-r1") - .addRecipient("foo-h2-r2")) - .addRoute(RouteSpec("foo-r1") + .addRecipient("foo-h2-r2"))) + .addRoute(std::move(RouteSpec("foo-r1") .addHop("foo-h1") - .addHop("foo-h2")) - .addRoute(RouteSpec("foo-r2") + .addHop("foo-h2"))) + .addRoute(std::move(RouteSpec("foo-r2") .addHop("foo-h2") - .addHop("foo-h1"))) + .addHop("foo-h1")))) .addTable(RoutingTableSpec("bar") - .addHop(HopSpec("bar-h1", "bar-h1-sel") + .addHop(std::move(HopSpec("bar-h1", "bar-h1-sel") .addRecipient("bar-h1-r1") - .addRecipient("bar-h1-r2")) - .addHop(HopSpec("bar-h2", "bar-h2-sel") + .addRecipient("bar-h1-r2"))) + .addHop(std::move(HopSpec("bar-h2", "bar-h2-sel") .addRecipient("bar-h2-r1") - .addRecipient("bar-h2-r2")) - .addRoute(RouteSpec("bar-r1") + .addRecipient("bar-h2-r2"))) + .addRoute(std::move(RouteSpec("bar-r1") .addHop("bar-h1") - .addHop("bar-h2")) - .addRoute(RouteSpec("bar-r2") + .addHop("bar-h2"))) + .addRoute(std::move(RouteSpec("bar-r2") .addHop("bar-h2") - .addHop("bar-h1"))); + .addHop("bar-h1")))); EXPECT_TRUE(testRouting(spec)); RoutingSpec specCopy = spec; @@ -184,20 +184,20 @@ Test::testConfigGeneration() .addHop(HopSpec("myhop1", "myselector1"))))); EXPECT_TRUE(testConfig(RoutingSpec().addTable(RoutingTableSpec("mytable1") .addHop(HopSpec("myhop1", "myselector1")) - .addRoute(RouteSpec("myroute1").addHop("myhop1"))))); + .addRoute(std::move(RouteSpec("myroute1").addHop("myhop1")))))); EXPECT_TRUE(testConfig(RoutingSpec().addTable(RoutingTableSpec("mytable1") .addHop(HopSpec("myhop1", "myselector1")) .addHop(HopSpec("myhop2", "myselector2")) - .addRoute(RouteSpec("myroute1").addHop("myhop1")) - .addRoute(RouteSpec("myroute2").addHop("myhop2")) - .addRoute(RouteSpec("myroute12").addHop("myhop1").addHop("myhop2"))))); + .addRoute(std::move(RouteSpec("myroute1").addHop("myhop1"))) + .addRoute(std::move(RouteSpec("myroute2").addHop("myhop2"))) + .addRoute(std::move(RouteSpec("myroute12").addHop("myhop1").addHop("myhop2")))))); EXPECT_TRUE(testConfig(RoutingSpec() .addTable(RoutingTableSpec("mytable1") .addHop(HopSpec("myhop1", "myselector1")) .addHop(HopSpec("myhop2", "myselector2")) - .addRoute(RouteSpec("myroute1").addHop("myhop1")) - .addRoute(RouteSpec("myroute2").addHop("myhop2")) - .addRoute(RouteSpec("myroute12").addHop("myhop1").addHop("myhop2"))) + .addRoute(std::move(RouteSpec("myroute1").addHop("myhop1"))) + .addRoute(std::move(RouteSpec("myroute2").addHop("myhop2"))) + .addRoute(std::move(RouteSpec("myroute12").addHop("myhop1").addHop("myhop2")))) .addTable(RoutingTableSpec("mytable2")))); EXPECT_EQUAL("routingtable[2]\n" @@ -222,11 +222,11 @@ Test::testConfigGeneration() .addTable(RoutingTableSpec("mytable1")) .addTable(RoutingTableSpec("mytable2") .addHop(HopSpec("myhop1", "myselector1")) - .addHop(HopSpec("myhop2", "myselector2").setIgnoreResult(true)) - .addHop(HopSpec("myhop1", "myselector3") + .addHop(std::move(HopSpec("myhop2", "myselector2").setIgnoreResult(true))) + .addHop(std::move(HopSpec("myhop1", "myselector3") .addRecipient("myrecipient1") - .addRecipient("myrecipient2")) - .addRoute(RouteSpec("myroute1").addHop("myhop1"))).toString()); + .addRecipient("myrecipient2"))) + .addRoute(std::move(RouteSpec("myroute1").addHop("myhop1")))).toString()); } bool diff --git a/messagebus/src/tests/simple-roundtrip/simple-roundtrip.cpp b/messagebus/src/tests/simple-roundtrip/simple-roundtrip.cpp index d3736f067c9..911c3d38e5e 100644 --- a/messagebus/src/tests/simple-roundtrip/simple-roundtrip.cpp +++ b/messagebus/src/tests/simple-roundtrip/simple-roundtrip.cpp @@ -18,9 +18,7 @@ RoutingSpec getRouting() { .addTable(RoutingTableSpec("Simple") .addHop(HopSpec("pxy", "test/pxy/session")) .addHop(HopSpec("dst", "test/dst/session")) - .addRoute(RouteSpec("test") - .addHop("pxy") - .addHop("dst"))); + .addRoute(std::move(RouteSpec("test").addHop("pxy").addHop("dst")))); } int @@ -47,47 +45,47 @@ Test::Main() ASSERT_TRUE(pxyNet.waitSlobrok("test/dst/session")); // send message on client - ss->send(SimpleMessage::UP(new SimpleMessage("test message")), "test"); + ss->send(std::make_unique<SimpleMessage>("test message"), "test"); // check message on proxy Message::UP msg = pxy.getMessage(); - ASSERT_TRUE(msg.get() != 0); + ASSERT_TRUE(msg); EXPECT_TRUE(msg->getProtocol() == SimpleProtocol::NAME); EXPECT_TRUE(msg->getType() == SimpleProtocol::MESSAGE); - EXPECT_TRUE(static_cast<SimpleMessage&>(*msg).getValue() == "test message"); + EXPECT_TRUE(dynamic_cast<SimpleMessage&>(*msg).getValue() == "test message"); // forward message on proxy - static_cast<SimpleMessage&>(*msg).setValue("test message pxy"); + dynamic_cast<SimpleMessage&>(*msg).setValue("test message pxy"); is->forward(std::move(msg)); // check message on server msg = dst.getMessage(); - ASSERT_TRUE(msg.get() != 0); + ASSERT_TRUE(msg); EXPECT_TRUE(msg->getProtocol() == SimpleProtocol::NAME); EXPECT_TRUE(msg->getType() == SimpleProtocol::MESSAGE); - EXPECT_TRUE(static_cast<SimpleMessage&>(*msg).getValue() == "test message pxy"); + EXPECT_TRUE(dynamic_cast<SimpleMessage&>(*msg).getValue() == "test message pxy"); // send reply on server - SimpleReply::UP sr(new SimpleReply("test reply")); + auto sr = std::make_unique<SimpleReply>("test reply"); msg->swapState(*sr); ds->reply(Reply::UP(sr.release())); // check reply on proxy Reply::UP reply = pxy.getReply(); - ASSERT_TRUE(reply.get() != 0); + ASSERT_TRUE(reply); EXPECT_TRUE(reply->getProtocol() == SimpleProtocol::NAME); EXPECT_TRUE(reply->getType() == SimpleProtocol::REPLY); - EXPECT_TRUE(static_cast<SimpleReply&>(*reply).getValue() == "test reply"); + EXPECT_TRUE(dynamic_cast<SimpleReply&>(*reply).getValue() == "test reply"); // forward reply on proxy - static_cast<SimpleReply&>(*reply).setValue("test reply pxy"); + dynamic_cast<SimpleReply&>(*reply).setValue("test reply pxy"); is->forward(std::move(reply)); // check reply on client reply = src.getReply(); - ASSERT_TRUE(reply.get() != 0); + ASSERT_TRUE(reply); EXPECT_TRUE(reply->getProtocol() == SimpleProtocol::NAME); EXPECT_TRUE(reply->getType() == SimpleProtocol::REPLY); - EXPECT_TRUE(static_cast<SimpleReply&>(*reply).getValue() == "test reply pxy"); + EXPECT_TRUE(dynamic_cast<SimpleReply&>(*reply).getValue() == "test reply pxy"); TEST_DONE(); } diff --git a/messagebus/src/tests/sourcesession/sourcesession.cpp b/messagebus/src/tests/sourcesession/sourcesession.cpp index 6ebbc68f965..a91c128b0b0 100644 --- a/messagebus/src/tests/sourcesession/sourcesession.cpp +++ b/messagebus/src/tests/sourcesession/sourcesession.cpp @@ -13,7 +13,6 @@ #include <vespa/messagebus/routing/routingspec.h> #include <vespa/messagebus/testlib/simplemessage.h> #include <vespa/messagebus/testlib/simpleprotocol.h> -#include <vespa/messagebus/testlib/simplereply.h> #include <vespa/messagebus/testlib/slobrok.h> #include <vespa/messagebus/testlib/testserver.h> #include <vespa/vespalib/testkit/testapp.h> @@ -29,7 +28,7 @@ struct DelayedHandler : public IMessageHandler DelayedHandler(MessageBus &mb, uint32_t d) : session(), delay(d) { session = mb.createDestinationSession("session", true, *this); } - ~DelayedHandler() { + ~DelayedHandler() override { session.reset(); } void handleMessage(Message::UP msg) override { @@ -45,14 +44,14 @@ RoutingSpec getRouting() { return RoutingSpec() .addTable(RoutingTableSpec("Simple") .addHop(HopSpec("dst", "dst/session")) - .addRoute(RouteSpec("dst").addHop("dst"))); + .addRoute(std::move(RouteSpec("dst").addHop("dst")))); } RoutingSpec getBadRouting() { return RoutingSpec() .addTable(RoutingTableSpec("Simple") .addHop(HopSpec("dst", "dst/session")) - .addRoute(RouteSpec("dst").addHop("dst"))); + .addRoute(std::move(RouteSpec("dst").addHop("dst")))); } bool waitQueueSize(RoutableQueue &queue, uint32_t size) { @@ -183,8 +182,8 @@ Test::testResendConnDown() RPCNetworkParams(slobrok.config())); src.mb.setupRouting(RoutingSpec().addTable(RoutingTableSpec(SimpleProtocol::NAME) .addHop(HopSpec("dst", "dst2/session")) - .addHop(HopSpec("pxy", "[All]").addRecipient("dst")) - .addRoute(RouteSpec("dst").addHop("pxy")))); + .addHop(std::move(HopSpec("pxy", "[All]").addRecipient("dst"))) + .addRoute(std::move(RouteSpec("dst").addHop("pxy"))))); RoutableQueue srcQ; SourceSession::UP ss = src.mb.createSourceSession(srcQ); @@ -243,7 +242,7 @@ Test::testIllegalRoute() while (srcQ.size() > 0) { Routable::UP routable = srcQ.dequeue(); ASSERT_TRUE(routable->isReply()); - Reply::UP r(static_cast<Reply*>(routable.release())); + Reply::UP r(dynamic_cast<Reply*>(routable.release())); EXPECT_EQUAL(1u, r->getNumErrors()); EXPECT_EQUAL((uint32_t)ErrorCode::NO_ADDRESS_FOR_SERVICE, r->getError(0).getCode()); string trace = r->getTrace().toString(); @@ -275,7 +274,7 @@ Test::testNoServices() while (srcQ.size() > 0) { Routable::UP routable = srcQ.dequeue(); ASSERT_TRUE(routable->isReply()); - Reply::UP r(static_cast<Reply*>(routable.release())); + Reply::UP r(dynamic_cast<Reply*>(routable.release())); EXPECT_TRUE(r->getNumErrors() == 1); EXPECT_TRUE(r->getError(0).getCode() == ErrorCode::NO_ADDRESS_FOR_SERVICE); string trace = r->getTrace().toString(); diff --git a/messagebus/src/tests/throttling/throttling.cpp b/messagebus/src/tests/throttling/throttling.cpp index a601845a5fc..3063e3ae8b5 100644 --- a/messagebus/src/tests/throttling/throttling.cpp +++ b/messagebus/src/tests/throttling/throttling.cpp @@ -4,7 +4,6 @@ #include <vespa/messagebus/destinationsession.h> #include <vespa/messagebus/dynamicthrottlepolicy.h> #include <vespa/messagebus/routablequeue.h> -#include <vespa/messagebus/routing/retrytransienterrorspolicy.h> #include <vespa/messagebus/routing/routingspec.h> #include <vespa/messagebus/sourcesession.h> #include <vespa/messagebus/sourcesessionparams.h> @@ -41,7 +40,7 @@ RoutingSpec getRouting() return RoutingSpec() .addTable(RoutingTableSpec("Simple") .addHop(HopSpec("dst", "dst/session")) - .addRoute(RouteSpec("dst").addHop("dst"))); + .addRoute(std::move(RouteSpec("dst").addHop("dst")))); } bool waitQueueSize(RoutableQueue &queue, uint32_t size) diff --git a/messagebus/src/tests/trace-roundtrip/trace-roundtrip.cpp b/messagebus/src/tests/trace-roundtrip/trace-roundtrip.cpp index 17d4071d411..7fbc3b096d6 100644 --- a/messagebus/src/tests/trace-roundtrip/trace-roundtrip.cpp +++ b/messagebus/src/tests/trace-roundtrip/trace-roundtrip.cpp @@ -12,8 +12,6 @@ #include <vespa/messagebus/testlib/receptor.h> #include <vespa/messagebus/sourcesessionparams.h> #include <vespa/messagebus/testlib/simplemessage.h> -#include <vespa/messagebus/testlib/simplereply.h> -#include <vespa/messagebus/testlib/simpleprotocol.h> using namespace mbus; @@ -81,7 +79,7 @@ RoutingSpec getRouting() { .addTable(RoutingTableSpec("Simple") .addHop(HopSpec("pxy", "test/pxy/session")) .addHop(HopSpec("dst", "test/dst/session")) - .addRoute(RouteSpec("test").addHop("pxy").addHop("dst"))); + .addRoute(std::move(RouteSpec("test").addHop("pxy").addHop("dst")))); } int diff --git a/messagebus/src/vespa/messagebus/configagent.cpp b/messagebus/src/vespa/messagebus/configagent.cpp index 0184f0456cf..60c6690f7e3 100644 --- a/messagebus/src/vespa/messagebus/configagent.cpp +++ b/messagebus/src/vespa/messagebus/configagent.cpp @@ -1,8 +1,8 @@ // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. -#include <vespa/messagebus/routing/routingspec.h> #include "configagent.h" #include "iconfighandler.h" +#include <vespa/messagebus/routing/routingspec.h> using namespace config; using namespace messagebus; @@ -18,26 +18,22 @@ ConfigAgent::configure(std::unique_ptr<MessagebusConfig> config) { const MessagebusConfig &cfg(*config); RoutingSpec spec; - using CFG = MessagebusConfig; - for (uint32_t t = 0; t < cfg.routingtable.size(); ++t) { - const CFG::Routingtable &table = cfg.routingtable[t]; + for (const auto & table : cfg.routingtable) { RoutingTableSpec tableSpec(table.protocol); - for (uint32_t h = 0; h < table.hop.size(); ++h) { - const CFG::Routingtable::Hop &hop = table.hop[h]; + for (const auto & hop : table.hop) { HopSpec hopSpec(hop.name, hop.selector); - for (uint32_t i = 0; i < hop.recipient.size(); ++i) { - hopSpec.addRecipient(hop.recipient[i]); + for (const auto & i : hop.recipient) { + hopSpec.addRecipient(i); } hopSpec.setIgnoreResult(hop.ignoreresult); - tableSpec.addHop(hopSpec); + tableSpec.addHop(std::move(hopSpec)); } - for (uint32_t r = 0; r < table.route.size(); ++r) { - const CFG::Routingtable::Route &route = table.route[r]; + for (const auto & route : table.route) { RouteSpec routeSpec(route.name); - for (uint32_t i = 0; i < route.hop.size(); ++i) { - routeSpec.addHop(route.hop[i]); + for (const auto & i : route.hop) { + routeSpec.addHop(i); } - tableSpec.addRoute(routeSpec); + tableSpec.addRoute(std::move(routeSpec)); } spec.addTable(tableSpec); } diff --git a/messagebus/src/vespa/messagebus/routing/hopspec.cpp b/messagebus/src/vespa/messagebus/routing/hopspec.cpp index 0ea996982ed..7c8980b33b1 100644 --- a/messagebus/src/vespa/messagebus/routing/hopspec.cpp +++ b/messagebus/src/vespa/messagebus/routing/hopspec.cpp @@ -14,27 +14,15 @@ HopSpec::HopSpec(const string &name, const string &selector) : _ignoreResult(false) { } -HopSpec::~HopSpec() {} +HopSpec::HopSpec(const HopSpec & rhs) = default; +HopSpec & HopSpec::operator=(const HopSpec & rhs) = default; +HopSpec::HopSpec(HopSpec && rhs) noexcept = default; +HopSpec & HopSpec::operator=(HopSpec && rhs) noexcept = default; +HopSpec::~HopSpec() = default; HopSpec & -HopSpec::addRecipients(const std::vector<string> &recipients) -{ - _recipients.insert(_recipients.end(), recipients.begin(), recipients.end()); - return *this; -} - -string -HopSpec::removeRecipient(uint32_t i) -{ - string ret = _recipients[i]; - _recipients.erase(_recipients.begin() + i); - return ret; -} - -HopSpec & -HopSpec::setIgnoreResult(bool ignoreResult) -{ - _ignoreResult = ignoreResult; +HopSpec::addRecipient(const string &recipient) { + _recipients.push_back(recipient); return *this; } diff --git a/messagebus/src/vespa/messagebus/routing/hopspec.h b/messagebus/src/vespa/messagebus/routing/hopspec.h index 2479537d7cc..fd2e4dfb431 100644 --- a/messagebus/src/vespa/messagebus/routing/hopspec.h +++ b/messagebus/src/vespa/messagebus/routing/hopspec.h @@ -31,6 +31,10 @@ public: * @param selector A string that represents the selector for this hop. */ HopSpec(const string &name, const string &selector); + HopSpec(const HopSpec & rhs); + HopSpec & operator=(const HopSpec & rhs); + HopSpec(HopSpec && rhs) noexcept; + HopSpec & operator=(HopSpec && rhs) noexcept; ~HopSpec(); /** @@ -38,28 +42,21 @@ public: * * @return The name. */ - const string &getName() const { return _name; } + [[nodiscard]] const string &getName() const { return _name; } /** * Returns the string selector that resolves the recipients of this hop. * * @return The selector. */ - const string &getSelector() const { return _selector; } - - /** - * Returns whether or not there are any recipients that the selector can choose from. - * - * @return True if there is at least one recipient. - */ - bool hasRecipients() const { return !_recipients.empty(); } + [[nodiscard]] const string &getSelector() const { return _selector; } /** * Returns the number of recipients that the selector can choose from. * * @return The number of recipients. */ - uint32_t getNumRecipients() const { return _recipients.size(); } + [[nodiscard]] uint32_t getNumRecipients() const { return _recipients.size(); } /** * Returns the recipients at the given index. @@ -67,7 +64,7 @@ public: * @param i The index of the recipient to return. * @return The recipient at the given index. */ - const string &getRecipient(uint32_t i) const { return _recipients[i]; } + [[nodiscard]] const string &getRecipient(uint32_t i) const { return _recipients[i]; } /** * Adds the given recipient to this. @@ -75,46 +72,14 @@ public: * @param recipient The recipient to add. * @return This, to allow chaining. */ - HopSpec &addRecipient(const string &recipient) { _recipients.push_back(recipient); return *this; } - - /** - * Adds the given recipients to this. - * - * @param recipients The recipients to add. - * @return This, to allow chaining. - */ - HopSpec &addRecipients(const std::vector<string> &recipients); - - /** - * Sets the recipient at the given index. - * - * @param i The index at which to set the recipient. - * @param recipient The recipient to set. - * @return This, to allow chaining. - */ - HopSpec &setRecipient(uint32_t i, const string &recipient) { _recipients[i] = recipient; return *this; } - - /** - * Removes the recipient at the given index. - * - * @param i The index of the recipient to remove. - * @return The removed recipient. - */ - string removeRecipient(uint32_t i); - - /** - * Clears the list of recipients that the selector may choose from. - * - * @return This, to allow chaining. - */ - HopSpec &clearRecipients() { _recipients.clear(); return *this; } + HopSpec &addRecipient(const string &recipient); /** * Returns whether or not to ignore the result when routing through this hop. * * @return True to ignore the result. */ - bool getIgnoreResult() const { return _ignoreResult; } + [[nodiscard]] bool getIgnoreResult() const { return _ignoreResult; } /** * Sets whether or not to ignore the result when routing through this hop. @@ -122,7 +87,10 @@ public: * @param ignoreResult Whether or not to ignore the result. * @return This, to allow chaining. */ - HopSpec &setIgnoreResult(bool ignoreResult); + HopSpec &setIgnoreResult(bool ignoreResult) { + _ignoreResult = ignoreResult; + return *this; + } /** * Appends the content of this to the given config string. @@ -137,7 +105,7 @@ public: * * @return The string. */ - string toString() const; + [[nodiscard]] string toString() const; /** * Implements the equality operator. diff --git a/messagebus/src/vespa/messagebus/routing/route.cpp b/messagebus/src/vespa/messagebus/routing/route.cpp index b393c2ccd6c..6be6788045c 100644 --- a/messagebus/src/vespa/messagebus/routing/route.cpp +++ b/messagebus/src/vespa/messagebus/routing/route.cpp @@ -4,15 +4,13 @@ namespace mbus { -Route::Route() : - _hops() -{ } +Route::Route() = default; Route::Route(std::vector<Hop> lst) : _hops(std::move(lst)) { } -Route::~Route() { } +Route::~Route() = default; Route & Route::addHop(Hop hop) @@ -36,13 +34,6 @@ Route::removeHop(uint32_t i) return ret; } -Route & -Route::clearHops() -{ - _hops.clear(); - return *this; -} - string Route::toString() const { string ret = ""; diff --git a/messagebus/src/vespa/messagebus/routing/route.h b/messagebus/src/vespa/messagebus/routing/route.h index d5839c128d1..43b76de4aeb 100644 --- a/messagebus/src/vespa/messagebus/routing/route.h +++ b/messagebus/src/vespa/messagebus/routing/route.h @@ -49,21 +49,21 @@ public: * * @param hops The hops to instantiate with. */ - Route(std::vector<Hop> hops); + explicit Route(std::vector<Hop> hops); /** * Returns whether or not there are any hops in this route. * * @return True if there is at least one hop. */ - bool hasHops() const { return !_hops.empty(); } + [[nodiscard]] bool hasHops() const { return !_hops.empty(); } /** * Returns the number of hops that make up this route. * * @return The number of hops. */ - uint32_t getNumHops() const { return _hops.size(); } + [[nodiscard]] uint32_t getNumHops() const { return _hops.size(); } /** * Returns the hop at the given index. @@ -79,7 +79,7 @@ public: * @param i The index of the hop to return. * @return The hop. */ - const Hop &getHop(uint32_t i) const { return _hops[i]; } + [[nodiscard]] const Hop &getHop(uint32_t i) const { return _hops[i]; } /** * Adds a hop to the list of hops that make up this route. @@ -107,25 +107,18 @@ public: Hop removeHop(uint32_t i); /** - * Clears the list of hops that make up this route. - * - * @return This, to allow chaining. - */ - Route &clearHops(); - - /** * Returns a string representation of this route. * * @return A string representation. */ - string toString() const; + [[nodiscard]] string toString() const; /** * Returns a string representation of this that can be debugged but not parsed. * * @return The debug string. */ - string toDebugString() const; + [[nodiscard]] string toDebugString() const; }; } // namespace mbus diff --git a/messagebus/src/vespa/messagebus/routing/routedirective.h b/messagebus/src/vespa/messagebus/routing/routedirective.h index 26cbebb6d6f..9fdbe0bbf6b 100644 --- a/messagebus/src/vespa/messagebus/routing/routedirective.h +++ b/messagebus/src/vespa/messagebus/routing/routedirective.h @@ -23,7 +23,7 @@ public: * * @param name The name of the route to insert. */ - RouteDirective(vespalib::stringref name); + explicit RouteDirective(vespalib::stringref name); ~RouteDirective() override; /** @@ -31,12 +31,12 @@ public: * * @return The name name. */ - const string &getName() const { return _name; } + [[nodiscard]] const string &getName() const { return _name; } - Type getType() const override { return TYPE_ROUTE; } - bool matches(const IHopDirective &dir) const override; - string toString() const override; - string toDebugString() const override; + [[nodiscard]] Type getType() const override { return TYPE_ROUTE; } + [[nodiscard]] bool matches(const IHopDirective &dir) const override; + [[nodiscard]] string toString() const override; + [[nodiscard]] string toDebugString() const override; }; } // mbus diff --git a/messagebus/src/vespa/messagebus/routing/routespec.cpp b/messagebus/src/vespa/messagebus/routing/routespec.cpp index cc1f1a9301d..00b8d7ed322 100644 --- a/messagebus/src/vespa/messagebus/routing/routespec.cpp +++ b/messagebus/src/vespa/messagebus/routing/routespec.cpp @@ -7,29 +7,29 @@ using vespalib::make_string; namespace mbus { -RouteSpec::RouteSpec(const string &name) : +RouteSpec::RouteSpec(const string &name) noexcept : _name(name), _hops() { } RouteSpec::RouteSpec(const RouteSpec &) = default; RouteSpec & RouteSpec::operator = (const RouteSpec &) = default; +RouteSpec::RouteSpec(RouteSpec &&) noexcept = default; +RouteSpec & RouteSpec::operator = (RouteSpec &&) noexcept = default; -RouteSpec::~RouteSpec() {} +RouteSpec::~RouteSpec() = default; RouteSpec & -RouteSpec::addHops(const std::vector<string> &hops) -{ - _hops.insert(_hops.end(), hops.begin(), hops.end()); +RouteSpec::addHop(const string &hop) { + _hops.push_back(hop); return *this; } -string -RouteSpec::removeHop(uint32_t i) -{ - string ret = _hops[i]; - _hops.erase(_hops.begin() + i); - return ret; + +RouteSpec & +RouteSpec::setHop(uint32_t i, const string &hop) { + _hops[i] = hop; + return *this; } void diff --git a/messagebus/src/vespa/messagebus/routing/routespec.h b/messagebus/src/vespa/messagebus/routing/routespec.h index 61853571b9e..273836c94d5 100644 --- a/messagebus/src/vespa/messagebus/routing/routespec.h +++ b/messagebus/src/vespa/messagebus/routing/routespec.h @@ -27,12 +27,11 @@ public: * * @param name A protocol-unique name for this route. */ - RouteSpec(const string &name); + explicit RouteSpec(const string &name) noexcept; RouteSpec(const RouteSpec &); RouteSpec & operator = (const RouteSpec &); - RouteSpec(RouteSpec &&) = default; - RouteSpec & operator = (RouteSpec &&) = default; - + RouteSpec(RouteSpec &&) noexcept; + RouteSpec & operator = (RouteSpec &&) noexcept; ~RouteSpec(); /** @@ -40,7 +39,7 @@ public: * * @return The name. */ - const string &getName() const { return _name; } + [[nodiscard]] const string &getName() const { return _name; } /** * Returns the hop name at the given index. @@ -48,21 +47,14 @@ public: * @param i The index of the hop to return. * @return The hop at the given index. */ - const string &getHop(uint32_t i) const { return _hops[i]; } - - /** - * Returns whether or not there are any hops in this route. - * - * @return True if there is at least one hop. - */ - bool hasHops() const { return !_hops.empty(); } + [[nodiscard]] const string &getHop(uint32_t i) const { return _hops[i]; } /** * Returns the number of hops that make up this route. * * @return The number of hops. */ - uint32_t getNumHops() const { return _hops.size(); } + [[nodiscard]] uint32_t getNumHops() const { return _hops.size(); } /** * Adds the given hop name to this. @@ -70,15 +62,7 @@ public: * @param hop The hop to add. * @return This, to allow chaining. */ - RouteSpec &addHop(const string &hop) { _hops.push_back(hop); return *this; } - - /** - * Adds the given hop names to this. - * - * @param hops The hops to add. - * @return This, to allow chaining. - */ - RouteSpec &addHops(const std::vector<string> &hops); + RouteSpec & addHop(const string &hop); /** * Sets the hop name for a given index. @@ -87,22 +71,7 @@ public: * @param hop The hop to set. * @return This, to allow chaining. */ - RouteSpec &setHop(uint32_t i, const string &hop) { _hops[i] = hop; return *this; } - - /** - * Removes the hop name at the given index. - * - * @param i The index of the hop to remove. - * @return The removed hop. - */ - string removeHop(uint32_t i); - - /** - * Clears the list of hops that make up this route. - * - * @return This, to allow chaining. - */ - RouteSpec &clearHops() { _hops.clear(); return *this; } + RouteSpec &setHop(uint32_t i, const string &hop); /** * Appends the content of this to the given config string. @@ -117,7 +86,7 @@ public: * * @return The string. */ - string toString() const; + [[nodiscard]] string toString() const; /** * Implements the equality operator. diff --git a/messagebus/src/vespa/messagebus/routing/routingcontext.cpp b/messagebus/src/vespa/messagebus/routing/routingcontext.cpp index 085820d5c3c..03451c3de56 100644 --- a/messagebus/src/vespa/messagebus/routing/routingcontext.cpp +++ b/messagebus/src/vespa/messagebus/routing/routingcontext.cpp @@ -8,12 +8,14 @@ namespace mbus { RoutingContext::RoutingContext(RoutingNode &node, uint32_t directive) : _node(node), + _context(), _directive(directive), - _consumableErrors(), _selectOnRetry(true), - _context() + _consumableErrors() { } +RoutingContext::~RoutingContext() = default; + bool RoutingContext::hasRecipients() const { @@ -60,19 +62,6 @@ RoutingContext::getMatchedRecipients(std::vector<Route> &ret) const } } -bool -RoutingContext::getSelectOnRetry() const -{ - return _selectOnRetry; -} - -RoutingContext & -RoutingContext::setSelectOnRetry(bool selectOnRetry) -{ - _selectOnRetry = selectOnRetry; - return *this; -} - const Route & RoutingContext::getRoute() const { @@ -85,12 +74,6 @@ RoutingContext::getHop() const return _node.getRoute().getHop(0); } -uint32_t -RoutingContext::getDirectiveIndex() const -{ - return _directive; -} - const PolicyDirective & RoutingContext::getDirective() const { @@ -109,25 +92,6 @@ RoutingContext::getHopSuffix() const return getHop().getSuffix(_directive); } -Context & -RoutingContext::getContext() -{ - return _context; -} - -const Context & -RoutingContext::getContext() const -{ - return _context; -} - -RoutingContext & -RoutingContext::setContext(const Context &ctx) -{ - _context = ctx; - return *this; -} - const Message & RoutingContext::getMessage() const { @@ -140,12 +104,6 @@ RoutingContext::trace(uint32_t level, const string ¬e) _node.getTrace().trace(level, note); } -bool -RoutingContext::hasReply() const -{ - return _node.hasReply(); -} - const Reply & RoutingContext::getReply() const { diff --git a/messagebus/src/vespa/messagebus/routing/routingcontext.h b/messagebus/src/vespa/messagebus/routing/routingcontext.h index d1323c35f22..3f85b8161ed 100644 --- a/messagebus/src/vespa/messagebus/routing/routingcontext.h +++ b/messagebus/src/vespa/messagebus/routing/routingcontext.h @@ -22,38 +22,38 @@ class Error; class RoutingContext { private: RoutingNode &_node; + Context _context; uint32_t _directive; - std::set<uint32_t> _consumableErrors; bool _selectOnRetry; - Context _context; + std::set<uint32_t> _consumableErrors; public: /** - * Convenience typedefs. - */ - using UP = std::unique_ptr<RoutingContext>; - - /** * Constructs a new routing context for a given routing node and hop. * * @param node The owning routing node. * @param directive The index to the policy directive of the hop. */ RoutingContext(RoutingNode &node, uint32_t directive); + RoutingContext(const RoutingContext &) = delete; + RoutingContext(RoutingContext &&) = delete; + RoutingContext & operator=(const RoutingContext &) = delete; + RoutingContext & operator=(RoutingContext &&) = delete; + ~RoutingContext(); /** * Returns whether or not this hop has any configured recipients. * * @return True if there is at least one recipient. */ - bool hasRecipients() const; + [[nodiscard]] bool hasRecipients() const; /** * Returns the number of configured recipients for this hop. * * @return The recipient count. */ - uint32_t getNumRecipients() const; + [[nodiscard]] uint32_t getNumRecipients() const; /** * Returns the configured recipient at the given index. @@ -61,14 +61,14 @@ public: * @param idx The index of the recipient to return. * @return The reipient at the given index. */ - const Route &getRecipient(uint32_t idx) const; + [[nodiscard]] const Route &getRecipient(uint32_t idx) const; /** * Returns all configured recipients for this hop. * * @return An unmodifiable list of recipients. */ - const std::vector<Route> &getAllRecipients() const; + [[nodiscard]] const std::vector<Route> &getAllRecipients() const; /** * Returns a list of all configured recipients whose first hop matches this. @@ -82,7 +82,7 @@ public: * * @return True to invoke {@link RoutingPolicy#select(RoutingContext)} on resend. */ - bool getSelectOnRetry() const; + [[nodiscard]] bool getSelectOnRetry() const { return _selectOnRetry; } /** * Sets whether or not the policy is required to reselect if resending occurs. @@ -90,35 +90,38 @@ public: * @param selectOnRetry The value to set. * @return This, to allow chaining. */ - RoutingContext &setSelectOnRetry(bool selectOnRetry); + RoutingContext &setSelectOnRetry(bool selectOnRetry) { + _selectOnRetry = selectOnRetry; + return *this; + } /** * Returns the route that contains the routing policy that spawned this. * * @return The route. */ - const Route &getRoute() const; + [[nodiscard]] const Route &getRoute() const; /** * Returns the hop that contains the routing policy that spawned this. * * @return The hop. */ - const Hop &getHop() const; + [[nodiscard]] const Hop &getHop() const; /** * Returns the index of the hop directive that spawned this. * * @return The directive index. */ - uint32_t getDirectiveIndex() const; + [[nodiscard]] uint32_t getDirectiveIndex() const { return _directive; } /** * Returns the policy directive that spawned this. * * @return The directive object. */ - const PolicyDirective &getDirective() const; + [[nodiscard]] const PolicyDirective &getDirective() const; /** * Returns the part of the route string that precedes the active policy directive. This is the same as calling @@ -126,7 +129,7 @@ public: * * @return The hop prefix. */ - string getHopPrefix() const; + [[nodiscard]] string getHopPrefix() const; /** * Returns the remainder of the route string immediately following the active policy directive. This is the same as @@ -134,21 +137,21 @@ public: * * @return The hop suffix. */ - string getHopSuffix() const; + [[nodiscard]] string getHopSuffix() const; /** * Returns the routing specific context object. * * @return The context. */ - Context &getContext(); + Context &getContext() { return _context; } /** * Returns a const reference to the routing specific context object. * * @return The context. */ - const Context &getContext() const; + [[nodiscard]] const Context &getContext() const { return _context; } /** * Sets a routing specific context object that will be available at merge(). @@ -156,14 +159,17 @@ public: * @param context An arbitrary object. * @return This, to allow chaining. */ - RoutingContext &setContext(const Context &ctx); + RoutingContext &setContext(const Context &ctx) { + _context = ctx; + return *this; + } /** * Returns the message being routed. * * @return The message. */ - const Message &getMessage() const; + [[nodiscard]] const Message &getMessage() const; /** * Adds a string to the trace of the message being routed. @@ -174,18 +180,11 @@ public: void trace(uint32_t level, const string ¬e); /** - * Returns whether or not a reply is available. - * - * @return True if a reply is set. - */ - bool hasReply() const; - - /** * Returns the reply generated by the associated routing policy. * * @return The reply. */ - const Reply &getReply() const; + [[nodiscard]] const Reply &getReply() const; /** * Sets the reply generated by the associated routing policy. @@ -226,14 +225,14 @@ public: * * @return True if there is at least one child. */ - bool hasChildren() const; + [[nodiscard]] bool hasChildren() const; /** * Returns the number of children the owning routing node has. * * @return The child count. */ - uint32_t getNumChildren() const; + [[nodiscard]] uint32_t getNumChildren() const; /** * Returns an iterator for the child routing nodes. @@ -263,7 +262,7 @@ public: * * @return The mirror api. */ - const slobrok::api::IMirrorAPI &getMirror() const; + [[nodiscard]] const slobrok::api::IMirrorAPI &getMirror() const; /** * Adds the given error code to the list of codes that the associated routing policy <u>may</u> consume. This is diff --git a/messagebus/src/vespa/messagebus/routing/routingnode.h b/messagebus/src/vespa/messagebus/routing/routingnode.h index d6d4da64ae4..02f2a9f7012 100644 --- a/messagebus/src/vespa/messagebus/routing/routingnode.h +++ b/messagebus/src/vespa/messagebus/routing/routingnode.h @@ -43,10 +43,10 @@ private: Reply::UP _reply; Route _route; IRoutingPolicy::SP _policy; - RoutingContext::UP _routingContext; - IServiceAddress::UP _serviceAddress; - bool _isActive; - bool _shouldRetry; + std::unique_ptr<RoutingContext> _routingContext; + IServiceAddress::UP _serviceAddress; + bool _isActive; + bool _shouldRetry; /** * Constructs a new instance of this class. This is the child node @@ -233,7 +233,7 @@ public: * Destructor. Frees up any allocated resources, namely all child nodes of * this. */ - ~RoutingNode(); + ~RoutingNode() override; /** * Discards this routing node. Invoking this will notify the parent {@link diff --git a/messagebus/src/vespa/messagebus/routing/routingnodeiterator.cpp b/messagebus/src/vespa/messagebus/routing/routingnodeiterator.cpp index 6d78f851f22..83ca8d37ae4 100644 --- a/messagebus/src/vespa/messagebus/routing/routingnodeiterator.cpp +++ b/messagebus/src/vespa/messagebus/routing/routingnodeiterator.cpp @@ -8,12 +8,6 @@ RoutingNodeIterator::RoutingNodeIterator(std::vector<RoutingNode*> &children) : _end(children.end()) { } -bool -RoutingNodeIterator::isValid() -{ - return _pos != _end; -} - RoutingNodeIterator & RoutingNodeIterator::next() { @@ -36,12 +30,6 @@ RoutingNodeIterator::getRoute() const return (*_pos)->getRoute(); } -bool -RoutingNodeIterator::hasReply() const -{ - return (*_pos)->hasReply(); -} - Reply::UP RoutingNodeIterator::removeReply() { diff --git a/messagebus/src/vespa/messagebus/routing/routingnodeiterator.h b/messagebus/src/vespa/messagebus/routing/routingnodeiterator.h index 25c1de777d1..a1612fdf0bf 100644 --- a/messagebus/src/vespa/messagebus/routing/routingnodeiterator.h +++ b/messagebus/src/vespa/messagebus/routing/routingnodeiterator.h @@ -22,14 +22,14 @@ public: * * @param children The list to iterate through. */ - RoutingNodeIterator(std::vector<RoutingNode*> &children); + explicit RoutingNodeIterator(std::vector<RoutingNode*> &children); /** * Returns whether or not this iterator is valid. * * @return True if we are still pointing to a valid entry. */ - bool isValid(); + [[nodiscard]] bool isValid() const { return _pos != _end; } /** * Steps to the next child in the map. @@ -51,14 +51,7 @@ public: * * @return The route. */ - const Route &getRoute() const; - - /** - * Returns whether or not a reply is set in the current child. - * - * @return True if a reply is available. - */ - bool hasReply() const; + [[nodiscard]] const Route &getRoute() const; /** * Removes and returns the reply of the current child. This is the correct way of reusing a reply of a @@ -73,7 +66,7 @@ public: * * @return The reply. */ - const Reply &getReplyRef() const; + [[nodiscard]] const Reply &getReplyRef() const; }; } // mbus diff --git a/messagebus/src/vespa/messagebus/routing/routingtablespec.cpp b/messagebus/src/vespa/messagebus/routing/routingtablespec.cpp index 328565a3ef7..6b20bd4af9f 100644 --- a/messagebus/src/vespa/messagebus/routing/routingtablespec.cpp +++ b/messagebus/src/vespa/messagebus/routing/routingtablespec.cpp @@ -14,24 +14,20 @@ RoutingTableSpec::RoutingTableSpec(const string &protocol) : RoutingTableSpec::RoutingTableSpec(const RoutingTableSpec&) = default; -RoutingTableSpec::~RoutingTableSpec() {} +RoutingTableSpec::~RoutingTableSpec() = default; RoutingTableSpec& RoutingTableSpec::operator=(const RoutingTableSpec&) = default; -HopSpec -RoutingTableSpec::removeHop(uint32_t i) -{ - HopSpec ret = _hops[i]; - _hops.erase(_hops.begin() + i); - return ret; +RoutingTableSpec & +RoutingTableSpec::addHop(HopSpec && hop) { + _hops.emplace_back(std::move(hop)); + return *this; } -RouteSpec -RoutingTableSpec::removeRoute(uint32_t i) -{ - RouteSpec ret = _routes[i]; - _routes.erase(_routes.begin() + i); - return ret; +RoutingTableSpec & +RoutingTableSpec::setHop(uint32_t i, HopSpec &&hop) { + _hops[i] = std::move(hop); + return *this; } void diff --git a/messagebus/src/vespa/messagebus/routing/routingtablespec.h b/messagebus/src/vespa/messagebus/routing/routingtablespec.h index eb0078d1767..63f1a68ade2 100644 --- a/messagebus/src/vespa/messagebus/routing/routingtablespec.h +++ b/messagebus/src/vespa/messagebus/routing/routingtablespec.h @@ -20,7 +20,7 @@ namespace mbus { */ class RoutingTableSpec { private: - string _protocol; + string _protocol; std::vector<HopSpec> _hops; std::vector<RouteSpec> _routes; @@ -30,7 +30,7 @@ public: * * @param protocol The name of the protocol that this belongs to. */ - RoutingTableSpec(const string &protocol); + explicit RoutingTableSpec(const string &protocol); RoutingTableSpec(const RoutingTableSpec&); RoutingTableSpec(RoutingTableSpec&&) noexcept = default; ~RoutingTableSpec(); @@ -42,21 +42,14 @@ public: * * @return The protocol name. */ - const string &getProtocol() const { return _protocol; } - - /** - * Returns whether or not there are any hop specs contained in this. - * - * @return True if there is at least one hop. - */ - bool hasHops() const { return !_hops.empty(); } + [[nodiscard]] const string &getProtocol() const { return _protocol; } /** * Returns the number of hops that are contained in this table. * * @return The number of hops. */ - uint32_t getNumHops() const { return _hops.size(); } + [[nodiscard]] uint32_t getNumHops() const { return _hops.size(); } /** * Returns the hop spec at the given index. @@ -72,7 +65,7 @@ public: * @param i The index of the hop to return. * @return The hop at the given position. */ - const HopSpec &getHop(uint32_t i) const { return _hops[i]; } + [[nodiscard]] const HopSpec &getHop(uint32_t i) const { return _hops[i]; } /** * Adds the given hop spec to this. @@ -80,7 +73,7 @@ public: * @param hop The hop to add. * @return This, to allow chaining. */ - RoutingTableSpec &addHop(const HopSpec &hop) { _hops.push_back(hop); return *this; } + RoutingTableSpec &addHop(HopSpec && hop); /** * Sets the hop spec at the given index. @@ -89,36 +82,14 @@ public: * @param hop The hop to set. * @return This, to allow chaining. */ - RoutingTableSpec &setHop(uint32_t i, const HopSpec &hop) { _hops[i] = hop; return *this; } - - /** - * Removes the hop spec at the given index. - * - * @param i The index of the hop to remove. - * @return The removed hop. - */ - HopSpec removeHop(uint32_t i); - - /** - * Clears the list of hop specs contained in this. - * - * @return This, to allow chaining. - */ - RoutingTableSpec &clearHops() { _hops.clear(); return *this; } - - /** - * Returns whether or not there are any route specs contained in this. - * - * @return True if there is at least one route. - */ - bool hasRoutes() const { return !_routes.empty(); } + RoutingTableSpec &setHop(uint32_t i, HopSpec &&hop); /** * Returns the number of route specs contained in this. * * @return The number of routes. */ - uint32_t getNumRoutes() const { return _routes.size(); } + [[nodiscard]] uint32_t getNumRoutes() const { return _routes.size(); } /** * Returns the route spec at the given index. @@ -134,7 +105,7 @@ public: * @param i The index of the route to return. * @return The route at the given index. */ - const RouteSpec &getRoute(uint32_t i) const { return _routes[i]; } + [[nodiscard]] const RouteSpec &getRoute(uint32_t i) const { return _routes[i]; } /** * Adds a route spec to this. @@ -142,7 +113,7 @@ public: * @param route The route to add. * @return This, to allow chaining. */ - RoutingTableSpec &addRoute(const RouteSpec &route) { _routes.push_back(route); return *this; } + RoutingTableSpec &addRoute(RouteSpec &&route) { _routes.emplace_back(std::move(route)); return *this; } /** * Sets the route spec at the given index. @@ -151,15 +122,7 @@ public: * @param route The route to set. * @return This, to allow chaining. */ - RoutingTableSpec &setRoute(uint32_t i, const RouteSpec &route) { _routes[i] = route; return *this; } - - /** - * Removes a route spec at a given index. - * - * @param i The index of the route to remove. - * @return The removed route. - */ - RouteSpec removeRoute(uint32_t i); + RoutingTableSpec &setRoute(uint32_t i, RouteSpec && route) { _routes[i] = std::move(route); return *this; } /** * Appends the content of this to the given config string. @@ -174,7 +137,7 @@ public: * * @return The string. */ - string toString() const; + [[nodiscard]] string toString() const; /** * Implements the equality operator. |