From a4efd988906825b7eda1a4eeef83005cc9ccb76e Mon Sep 17 00:00:00 2001 From: Stephen Seo Date: Tue, 17 Dec 2019 19:12:54 +0900 Subject: [PATCH] Replace "poor man's optional" with std::unique_ptr --- src/TSLQueue.hpp | 132 +++++++++++--------------------------- src/UDPConnection.cpp | 106 +++++++++++++++--------------- src/test/TestTSLQueue.cpp | 82 ++++++++++++++--------- 3 files changed, 139 insertions(+), 181 deletions(-) diff --git a/src/TSLQueue.hpp b/src/TSLQueue.hpp index 7ef865c..8ac0371 100644 --- a/src/TSLQueue.hpp +++ b/src/TSLQueue.hpp @@ -23,40 +23,14 @@ class TSLQueue { TSLQueue(TSLQueue &&other); TSLQueue &operator=(TSLQueue &&other); - // in place of std::optional which is only available in C++17 - struct Entry { - Entry(); - Entry(const T& value); - Entry(T&& rvalue); - - // enable copy - Entry(const Entry& other) = default; - Entry &operator =(const Entry& other) = default; - - // enable move - Entry(Entry&& other) = default; - Entry &operator =(Entry&& other) = default; - - enum Type { - NONE, - SOME - } type; - T value; - - bool has_value() const; - operator bool () const; - T& operator *(); - const T& operator *() const; - }; - void push(const T &data); bool push_nb(const T &data); - Entry top(); - Entry top_nb(); + std::unique_ptr top(); + std::unique_ptr top_nb(); bool pop(); - Entry top_and_pop(); - Entry top_and_pop_and_empty(bool *isEmpty); - Entry top_and_pop_and_rsize(unsigned long *rsize); + std::unique_ptr top_and_pop(); + std::unique_ptr top_and_pop_and_empty(bool *isEmpty); + std::unique_ptr top_and_pop_and_rsize(unsigned long *rsize); void clear(); bool empty(); @@ -93,7 +67,7 @@ class TSLQueue { unsigned long *msize); ~TSLQIter(); - Entry current(); + std::unique_ptr current(); bool next(); bool prev(); bool remove(); @@ -149,44 +123,6 @@ TSLQueue & TSLQueue::operator=(TSLQueue &&other) { msize = std::move(other.msize); } -template -TSLQueue::Entry::Entry() : - type(Type::NONE), - value() -{} - -template -TSLQueue::Entry::Entry(const T& value) : - type(Type::SOME), - value(value) -{} - -template -TSLQueue::Entry::Entry(T&& value) : - type(Type::SOME), - value(std::forward(value)) -{} - -template -bool TSLQueue::Entry::has_value() const { - return type == Type::SOME; -} - -template -TSLQueue::Entry::operator bool() const { - return has_value(); -} - -template -T& TSLQueue::Entry::operator *() { - return value; -} - -template -const T& TSLQueue::Entry::operator *() const { - return value; -} - template void TSLQueue::push(const T &data) { std::lock_guard lock(mutex); @@ -226,29 +162,29 @@ bool TSLQueue::push_nb(const T &data) { } template -typename TSLQueue::Entry TSLQueue::top() { +std::unique_ptr TSLQueue::top() { std::lock_guard lock(mutex); + std::unique_ptr result; if(head->next != tail) { assert(head->next->data); - return Entry(*head->next->data.get()); - } else { - return Entry(); + result = std::unique_ptr(new T); + *result = *head->next->data; } + return result; } template -typename TSLQueue::Entry TSLQueue::top_nb() { +std::unique_ptr TSLQueue::top_nb() { + std::unique_ptr result; if(mutex.try_lock()) { - Entry ret; if(head->next != tail) { assert(head->next->data); - ret = Entry(*head->next->data.get()); + result = std::unique_ptr(new T); + *result = *head->next->data; } mutex.unlock(); - return ret; - } else { - return Entry(); } + return result; } template @@ -268,12 +204,13 @@ bool TSLQueue::pop() { } template -typename TSLQueue::Entry TSLQueue::top_and_pop() { - Entry ret; +std::unique_ptr TSLQueue::top_and_pop() { + std::unique_ptr result; std::lock_guard lock(mutex); if(head->next != tail) { assert(head->next->data); - ret = Entry(*head->next->data.get()); + result = std::unique_ptr(new T); + *result = *head->next->data; auto& newNext = head->next->next; newNext->prev = head; @@ -281,12 +218,12 @@ typename TSLQueue::Entry TSLQueue::top_and_pop() { assert(msize > 0); --msize; } - return ret; + return result; } template -typename TSLQueue::Entry TSLQueue::top_and_pop_and_empty(bool *isEmpty) { - Entry ret; +std::unique_ptr TSLQueue::top_and_pop_and_empty(bool *isEmpty) { + std::unique_ptr result; std::lock_guard lock(mutex); if(head->next == tail) { if(isEmpty) { @@ -294,7 +231,8 @@ typename TSLQueue::Entry TSLQueue::top_and_pop_and_empty(bool *isEmpty) { } } else { assert(head->next->data); - ret = Entry(*head->next->data.get()); + result = std::unique_ptr(new T); + *result = *head->next->data; auto& newNext = head->next->next; newNext->prev = head; @@ -306,12 +244,12 @@ typename TSLQueue::Entry TSLQueue::top_and_pop_and_empty(bool *isEmpty) { *isEmpty = head->next == tail; } } - return ret; + return result; } template -typename TSLQueue::Entry TSLQueue::top_and_pop_and_rsize(unsigned long *rsize) { - Entry ret; +std::unique_ptr TSLQueue::top_and_pop_and_rsize(unsigned long *rsize) { + std::unique_ptr result; std::lock_guard lock(mutex); if(head->next == tail) { if(rsize) { @@ -319,7 +257,8 @@ typename TSLQueue::Entry TSLQueue::top_and_pop_and_rsize(unsigned long *rs } } else { assert(head->next->data); - ret = Entry(*head->next->data.get()); + result = std::unique_ptr(new T); + *result = *head->next->data; auto& newNext = head->next->next; newNext->prev = head; @@ -331,7 +270,7 @@ typename TSLQueue::Entry TSLQueue::top_and_pop_and_rsize(unsigned long *rs *rsize = msize; } } - return ret; + return result; } template @@ -382,14 +321,15 @@ TSLQueue::TSLQIter::~TSLQIter() { } template -typename TSLQueue::Entry TSLQueue::TSLQIter::current() { +std::unique_ptr TSLQueue::TSLQIter::current() { + std::unique_ptr result; std::shared_ptr currentNode = this->currentNode.lock(); assert(currentNode); if(currentNode->isNormal()) { - return Entry(*currentNode->data.get()); - } else { - return Entry(); + result = std::unique_ptr(new T); + *result = *currentNode->data; } + return result; } template diff --git a/src/UDPConnection.cpp b/src/UDPConnection.cpp index 60a7ecb..8ad59c8 100644 --- a/src/UDPConnection.cpp +++ b/src/UDPConnection.cpp @@ -264,8 +264,8 @@ void UDPC::Context::update_impl() { // handle internalEvents do { auto optE = internalEvents.top_and_pop(); - if(optE.has_value()) { - switch(optE.value.type) { + if(optE) { + switch(optE->type) { case UDPC_ET_REQUEST_CONNECT: { unsigned char *sk = nullptr; @@ -277,11 +277,11 @@ void UDPC::Context::update_impl() { UDPC::ConnectionData newCon( false, this, - optE.value.conId.addr, - optE.value.conId.scope_id, - optE.value.conId.port, + optE->conId.addr, + optE->conId.scope_id, + optE->conId.port, #ifdef UDPC_LIBSODIUM_ENABLED - flags.test(2) && optE.value.v.enableLibSodium != 0, + flags.test(2) && optE->v.enableLibSodium != 0, sk, pk); #else false, @@ -292,9 +292,9 @@ void UDPC::Context::update_impl() { UDPC_LoggingType::UDPC_ERROR, "Failed to init ConnectionData instance (libsodium " "init fail) while client establishing connection with ", - UDPC_atostr((UDPC_HContext)this, optE.value.conId.addr), + UDPC_atostr((UDPC_HContext)this, optE->conId.addr), " port ", - optE.value.conId.port); + optE->conId.port); continue; } newCon.sent = std::chrono::steady_clock::now() - UDPC::INIT_PKT_INTERVAL_DT; @@ -314,34 +314,34 @@ void UDPC::Context::update_impl() { #endif } - if(conMap.find(optE.value.conId) == conMap.end()) { + if(conMap.find(optE->conId) == conMap.end()) { conMap.insert(std::make_pair( - optE.value.conId, + optE->conId, std::move(newCon))); - auto addrConIter = addrConMap.find(optE.value.conId.addr); + auto addrConIter = addrConMap.find(optE->conId.addr); if(addrConIter == addrConMap.end()) { auto insertResult = addrConMap.insert(std::make_pair( - optE.value.conId.addr, + optE->conId.addr, std::unordered_set{})); assert(insertResult.second && "new connection insert into addrConMap must not fail"); addrConIter = insertResult.first; } - addrConIter->second.insert(optE.value.conId); + addrConIter->second.insert(optE->conId); UDPC_CHECK_LOG(this, UDPC_LoggingType::UDPC_INFO, "Client initiating connection to ", - UDPC_atostr((UDPC_HContext)this, optE.value.conId.addr), + UDPC_atostr((UDPC_HContext)this, optE->conId.addr), " port ", - optE.value.conId.port, + optE->conId.port, " ..."); } else { UDPC_CHECK_LOG(this, UDPC_LoggingType::UDPC_WARNING, "Client initiate connection, already connected to peer ", - UDPC_atostr((UDPC_HContext)this, optE.value.conId.addr), + UDPC_atostr((UDPC_HContext)this, optE->conId.addr), " port ", - optE.value.conId.port); + optE->conId.port); } } break; @@ -358,9 +358,9 @@ void UDPC::Context::update_impl() { UDPC::ConnectionData newCon( false, this, - optE.value.conId.addr, - optE.value.conId.scope_id, - optE.value.conId.port, + optE->conId.addr, + optE->conId.scope_id, + optE->conId.port, #ifdef UDPC_LIBSODIUM_ENABLED true, sk, pk); @@ -368,18 +368,18 @@ void UDPC::Context::update_impl() { false, sk, pk); assert(!"compiled without libsodium support"); - delete[] optE.value.v.pk; + delete[] optE->v.pk; break; #endif if(newCon.flags.test(5)) { - delete[] optE.value.v.pk; + delete[] optE->v.pk; UDPC_CHECK_LOG(this, UDPC_LoggingType::UDPC_ERROR, "Failed to init ConnectionData instance (libsodium " "init fail) while client establishing connection with ", - UDPC_atostr((UDPC_HContext)this, optE.value.conId.addr), + UDPC_atostr((UDPC_HContext)this, optE->conId.addr), " port ", - optE.value.conId.port); + optE->conId.port); continue; } newCon.sent = std::chrono::steady_clock::now() - UDPC::INIT_PKT_INTERVAL_DT; @@ -401,48 +401,48 @@ void UDPC::Context::update_impl() { // set peer public key std::memcpy( newCon.peer_pk, - optE.value.v.pk, + optE->v.pk, crypto_sign_PUBLICKEYBYTES); newCon.flags.set(7); } - delete[] optE.value.v.pk; + delete[] optE->v.pk; - if(conMap.find(optE.value.conId) == conMap.end()) { + if(conMap.find(optE->conId) == conMap.end()) { conMap.insert(std::make_pair( - optE.value.conId, + optE->conId, std::move(newCon))); - auto addrConIter = addrConMap.find(optE.value.conId.addr); + auto addrConIter = addrConMap.find(optE->conId.addr); if(addrConIter == addrConMap.end()) { auto insertResult = addrConMap.insert(std::make_pair( - optE.value.conId.addr, + optE->conId.addr, std::unordered_set{})); assert(insertResult.second && "new connection insert into addrConMap must not fail"); addrConIter = insertResult.first; } - addrConIter->second.insert(optE.value.conId); + addrConIter->second.insert(optE->conId); UDPC_CHECK_LOG(this, UDPC_LoggingType::UDPC_INFO, "Client initiating connection to ", - UDPC_atostr((UDPC_HContext)this, optE.value.conId.addr), + UDPC_atostr((UDPC_HContext)this, optE->conId.addr), " port ", - optE.value.conId.port, + optE->conId.port, " ..."); } else { UDPC_CHECK_LOG(this, UDPC_LoggingType::UDPC_WARNING, "Client initiate connection, already connected to peer ", - UDPC_atostr((UDPC_HContext)this, optE.value.conId.addr), + UDPC_atostr((UDPC_HContext)this, optE->conId.addr), " port ", - optE.value.conId.port); + optE->conId.port); } } break; case UDPC_ET_REQUEST_DISCONNECT: - if(optE.value.v.dropAllWithAddr != 0) { + if(optE->v.dropAllWithAddr != 0) { // drop all connections with same address - auto addrConIter = addrConMap.find(optE.value.conId.addr); + auto addrConIter = addrConMap.find(optE->conId.addr); if(addrConIter != addrConMap.end()) { for(auto identIter = addrConIter->second.begin(); identIter != addrConIter->second.end(); @@ -463,14 +463,14 @@ void UDPC::Context::update_impl() { } } else { // drop only specific connection with addr and port - auto iter = conMap.find(optE.value.conId); + auto iter = conMap.find(optE->conId); if(iter != conMap.end()) { if(iter->second.flags.test(4)) { idMap.erase(iter->second.id); } - auto addrConIter = addrConMap.find(optE.value.conId.addr); + auto addrConIter = addrConMap.find(optE->conId.addr); if(addrConIter != addrConMap.end()) { - addrConIter->second.erase(optE.value.conId); + addrConIter->second.erase(optE->conId); if(addrConIter->second.empty()) { addrConMap.erase(addrConIter); } @@ -607,18 +607,18 @@ void UDPC::Context::update_impl() { while(true) { auto next = sendIter.current(); if(next) { - auto iter = conMap.find(next.value.receiver); + auto iter = conMap.find(next->receiver); if(iter != conMap.end()) { if(iter->second.sendPkts.size() >= UDPC_QUEUED_PKTS_MAX_SIZE) { - if(notQueued.find(next.value.receiver) == notQueued.end()) { - notQueued.insert(next.value.receiver); + if(notQueued.find(next->receiver) == notQueued.end()) { + notQueued.insert(next->receiver); UDPC_CHECK_LOG(this, UDPC_LoggingType::UDPC_DEBUG, "Not queueing packet to ", UDPC_atostr((UDPC_HContext)this, - next.value.receiver.addr), + next->receiver.addr), ", port = ", - next.value.receiver.port, + next->receiver.port, ", connection's queue reached max size"); } if(sendIter.next()) { @@ -627,24 +627,24 @@ void UDPC::Context::update_impl() { break; } } - iter->second.sendPkts.push_back(next.value); + iter->second.sendPkts.push_back(*next); if(sendIter.remove()) { continue; } else { break; } } else { - if(dropped.find(next.value.receiver) == dropped.end()) { + if(dropped.find(next->receiver) == dropped.end()) { UDPC_CHECK_LOG(this, UDPC_LoggingType::UDPC_WARNING, "Dropped queued packets to ", UDPC_atostr( (UDPC_HContext)this, - next.value.receiver.addr), + next->receiver.addr), ", port = ", - next.value.receiver.port, + next->receiver.port, " due to connection not existing"); - dropped.insert(next.value.receiver); + dropped.insert(next->receiver); } if(sendIter.remove()) { continue; @@ -1929,8 +1929,8 @@ void UDPC_destroy(UDPC_HContext ctx) { #endif while(!UDPC_ctx->internalEvents.empty()) { auto optE = UDPC_ctx->internalEvents.top_and_pop(); - if(optE.has_value() && optE.value.type == UDPC_ET_REQUEST_CONNECT_PK) { - delete[] optE.value.v.pk; + if(optE && optE->type == UDPC_ET_REQUEST_CONNECT_PK) { + delete[] optE->v.pk; } } UDPC_ctx->_contextIdentifier = 0; @@ -2147,7 +2147,7 @@ UDPC_Event UDPC_get_event(UDPC_HContext ctx, unsigned long *remaining) { auto optE = c->externalEvents.top_and_pop_and_rsize(remaining); if(optE) { - return optE.value; + return *optE; } else { return UDPC_Event{UDPC_ET_NONE, UDPC_create_id_anyaddr(0), 0}; } diff --git a/src/test/TestTSLQueue.cpp b/src/test/TestTSLQueue.cpp index 4bb3647..f579712 100644 --- a/src/test/TestTSLQueue.cpp +++ b/src/test/TestTSLQueue.cpp @@ -8,7 +8,7 @@ TEST(TSLQueue, PushTopPopSize) { TSLQueue q; - EXPECT_FALSE(q.top().has_value()); + EXPECT_FALSE(q.top()); for(int i = 0; i < 10; ++i) { EXPECT_EQ(i, q.size()); @@ -17,8 +17,8 @@ TEST(TSLQueue, PushTopPopSize) { for(int i = 0; i < 10; ++i) { auto v = q.top(); - ASSERT_TRUE(v.has_value()); - EXPECT_EQ(v.value, i); + ASSERT_TRUE(v); + EXPECT_EQ(*v, i); EXPECT_EQ(10 - i, q.size()); EXPECT_TRUE(q.pop()); } @@ -37,21 +37,21 @@ TEST(TSLQueue, PushNB_TopNB_TopAndPop_Size) { for(int i = 0; i < 10; ++i) { auto v = q.top_nb(); - ASSERT_TRUE(v.has_value()); - EXPECT_EQ(v.value, i); + ASSERT_TRUE(v); + EXPECT_EQ(*v, i); EXPECT_EQ(q.size(), 10 - i); v = q.top_and_pop(); - ASSERT_TRUE(v.has_value()); - EXPECT_EQ(v.value, i); + ASSERT_TRUE(v); + EXPECT_EQ(*v, i); } { auto v = q.top_nb(); - ASSERT_FALSE(v.has_value()); + ASSERT_FALSE(v); } { auto v = q.top_and_pop(); - ASSERT_FALSE(v.has_value()); + ASSERT_FALSE(v); } EXPECT_EQ(q.size(), 0); } @@ -68,8 +68,8 @@ TEST(TSLQueue, Push_TopAndPopAndEmpty_Size) { for(int i = 0; i < 10; ++i) { EXPECT_EQ(q.size(), 10 - i); auto v = q.top_and_pop_and_empty(&isEmpty); - ASSERT_TRUE(v.has_value()); - EXPECT_EQ(v.value, i); + ASSERT_TRUE(v); + EXPECT_EQ(*v, i); EXPECT_EQ(i == 9, isEmpty); } EXPECT_EQ(q.size(), 0); @@ -109,9 +109,9 @@ TEST(TSLQueue, Concurrent) { for(int i = 0; i < 100; ++i) { EXPECT_EQ(q.size(), 100 - i); auto v = q.top_and_pop(); - ASSERT_TRUE(v.has_value()); - EXPECT_GE(v.value, 0); - EXPECT_LE(v.value, 100); + ASSERT_TRUE(v); + EXPECT_GE(*v, 0); + EXPECT_LE(*v, 100); EXPECT_EQ(i == 99, q.empty()); } EXPECT_EQ(q.size(), 0); @@ -130,8 +130,8 @@ TEST(TSLQueue, Iterator) { auto iter = q.begin(); int i = 0; auto op = iter.current(); - while(op.has_value()) { - EXPECT_EQ(op.value, i++); + while(op) { + EXPECT_EQ(*op, i++); if(i < 10) { EXPECT_TRUE(iter.next()); } else { @@ -143,13 +143,13 @@ TEST(TSLQueue, Iterator) { // test that lock is held by iterator EXPECT_FALSE(q.push_nb(10)); op = q.top_nb(); - EXPECT_FALSE(op.has_value()); + EXPECT_FALSE(op); // backwards iteration EXPECT_TRUE(iter.prev()); op = iter.current(); - while(op.has_value()) { - EXPECT_EQ(op.value, --i); + while(op) { + EXPECT_EQ(*op, --i); if(i > 0) { EXPECT_TRUE(iter.prev()); } else { @@ -168,23 +168,23 @@ TEST(TSLQueue, Iterator) { EXPECT_TRUE(iter.remove()); auto op = iter.current(); - EXPECT_TRUE(op.has_value()); - EXPECT_EQ(op.value, 4); + EXPECT_TRUE(op); + EXPECT_EQ(*op, 4); EXPECT_TRUE(iter.prev()); op = iter.current(); - EXPECT_TRUE(op.has_value()); - EXPECT_EQ(op.value, 2); + EXPECT_TRUE(op); + EXPECT_EQ(*op, 2); } EXPECT_EQ(q.size(), 9); // check that "3" was removed from queue int i = 0; - TSLQueue::Entry op; + std::unique_ptr op; while(!q.empty()) { op = q.top(); - EXPECT_TRUE(op.has_value()); - EXPECT_EQ(i++, op.value); + EXPECT_TRUE(op); + EXPECT_EQ(i++, *op); if(i == 3) { ++i; } @@ -205,8 +205,8 @@ TEST(TSLQueue, Iterator) { i = 1; while(!q.empty()) { op = q.top(); - EXPECT_TRUE(op.has_value()); - EXPECT_EQ(i++, op.value); + EXPECT_TRUE(op); + EXPECT_EQ(i++, *op); EXPECT_TRUE(q.pop()); } @@ -221,8 +221,8 @@ TEST(TSLQueue, Iterator) { while(true) { EXPECT_TRUE(iter.next()); op = iter.current(); - EXPECT_TRUE(op.has_value()); - if(op.value == 3) { + EXPECT_TRUE(op); + if(*op == 3) { EXPECT_FALSE(iter.remove()); break; } @@ -232,11 +232,29 @@ TEST(TSLQueue, Iterator) { i = 0; while(!q.empty()) { op = q.top(); - EXPECT_TRUE(op.has_value()); - EXPECT_EQ(i++, op.value); + EXPECT_TRUE(op); + EXPECT_EQ(i++, *op); EXPECT_TRUE(q.pop()); if(i == 3) { EXPECT_TRUE(q.empty()); } } } + +TEST(TSLQueue, TempToNew) { + TSLQueue q; + + q.push(1234); + q.push(5678); + + auto getValue = [] (TSLQueue *q) -> int { + auto uptr = q->top_and_pop(); + return *uptr; + }; + int value; + + value = getValue(&q); + EXPECT_EQ(1234, value); + value = getValue(&q); + EXPECT_EQ(5678, value); +}