Remove unreachable code #535
Conversation
This was noticed by MSVC in a debug build which produced the following
warning:
warning C4702: unreachable code
| @@ -108,7 +108,6 @@ CassError cass_execution_profile_set_latency_aware_routing_settings( | |||
| CassError cass_execution_profile_set_whitelist_filtering(CassExecProfile* profile, | |||
| const char* hosts) { | |||
| return cass_execution_profile_set_whitelist_filtering_n(profile, hosts, SAFE_STRLEN(hosts)); | |||
| return CASS_OK; | |||
There was a problem hiding this comment.
This one seems like a good idea on it's face
| @@ -120,9 +117,6 @@ class MPMCQueue : public Allocated { | |||
| pos = head_.load(MEMORY_ORDER_RELAXED); | |||
| } | |||
| } | |||
|
|
|||
| // never taken | |||
| return false; | |||
There was a problem hiding this comment.
These two seemed a bit more perplexing to me. I'm assuming they exist to make code analysis happy but I'm wondering if maybe it isn't better to do something like the following:
diff --git a/src/mpmc_queue.hpp b/src/mpmc_queue.hpp
index f6cd146c..53abdf5d 100644
--- a/src/mpmc_queue.hpp
+++ b/src/mpmc_queue.hpp
@@ -69,11 +69,7 @@ public:
// weak compare is faster, but can return spurious results
// which in this instance is OK, because it's in the loop
if (tail_.compare_exchange_weak(pos, pos + 1, MEMORY_ORDER_RELAXED)) {
- // set the data
- node->data = data;
- // increment the sequence so that the tail knows it's accessible
- node->seq.store(pos + 1, MEMORY_ORDER_RELEASE);
- return true;
+ break;
}
} else if (dif < 0) {
// if seq is less than head seq then it means this slot is
@@ -85,8 +81,11 @@ public:
}
}
- // never taken
- return false;
+ // set the data
+ node->data = data;
+ // increment the sequence so that the tail knows it's accessible
+ node->seq.store(pos + 1, MEMORY_ORDER_RELEASE);
+ return true;
}
bool dequeue(T& data) {
@@ -104,12 +103,7 @@ public:
// weak compare is faster, but can return spurious results
// which in this instance is OK, because it's in the loop
if (head_.compare_exchange_weak(pos, pos + 1, MEMORY_ORDER_RELAXED)) {
- // set the output
- data = node->data;
- // set the sequence to what the head sequence should be next
- // time around
- node->seq.store(pos + mask_ + 1, MEMORY_ORDER_RELEASE);
- return true;
+ break;
}
} else if (dif < 0) {
// if seq is less than head seq then it means this slot is
@@ -121,8 +115,12 @@ public:
}
}
- // never taken
- return false;
+ // set the output
+ data = node->data;
+ // set the sequence to what the head sequence should be next
+ // time around
+ node->seq.store(pos + mask_ + 1, MEMORY_ORDER_RELEASE);
+ return true;
}
bool is_empty() const { This change should be logically equivalent to the previous impl. It also brings us closer to the original implementation.
Whaddya think @SeverinLeonhardt ?
There was a problem hiding this comment.
My change is definitely to make code analysis happy, so maybe either way some compiler will complain.
Your suggested changes look good to me and nicely work around it. Actually I'd say this code is even better structured. If I understand GitHub right you should be able to push commits to the PR. Let me know if that doesn't work, then I'll add it.
There was a problem hiding this comment.
If I'm reading these docs right that's something that can be setup when the PR is created. Honestly it's probably faster/easier for you to incorporate the changes directly in another commit @SeverinLeonhardt. If that works for you let's just go with that; I'll take another look once the changes are in.
There was a problem hiding this comment.
@absurdfarce Sorry for the delay, commit is added to the PR now. I've also used the patched version to run some tests locally and it seemed to all work fine.
As suggested by @absurdfarce this change keeps finding a slot in the loop but moves the use of the slot past the loop. That way there is a return after the loop which should make clear to any code analysis that this function will always return something.
|
Apologies for the delay in getting back to you on this (and your other PRs) @SeverinLeonhardt. I'm trying to juggle lots of things at once and probably not doing a very good job with any of them. :( Your last round of changes look good, so I think we're ready to call this done. I'm going to hold off on merging it for a bit; we're working on some infrastructure changes here which have our CI setup in something of a mess at the moment. Once that gets sorted out I'll try to get this merged as quickly as possible. Thanks again! |
|
Regrettably wasn't able to include this (and your other changes) in 2.17.0 @SeverinLeonhardt but we're gearing up for another C++ driver release this quarter and I'd definitely like for these changes to get in; you've been waiting long enough as-is 🤦 . This one was absolutely ready to go so I'm putting it in now; I'll check the other two but IIRC they were both pretty close (or there already) as well. Many, many thanks for your patience! |
This was noticed by MSVC in a debug build which produced the following warning: