Use early return to clean up ClientImplBase::Session::send_message() #4908
Conversation
This should be identical to the original version, just easier to read.
| REALM_ASSERT(m_upload_progress.client_version <= m_upload_target_version); | ||
| REALM_ASSERT(m_upload_target_version <= m_last_version_available); | ||
| bool need_upload = (m_upload_target_version > m_upload_progress.client_version); | ||
| if (REALM_UNLIKELY(need_upload) && REALM_LIKELY(m_allow_upload)) |
There was a problem hiding this comment.
Yes, I know that this means that it is UNLIKELY that we will do anything at all in this function. That seems incorrect, but does match the old code 🤷
There was a problem hiding this comment.
Do we think that all these LIKELY hints are helping us? At the very least, I don't think we can know whether this is likely or unlikely and would be included to remove this hint.
There was a problem hiding this comment.
Anyway - I would assume that this code is not super performance critical, so are these hints more noisy than helpful?
There was a problem hiding this comment.
I'm happy to remove that nonsense. I'll update the patch on Wednesday
| } | ||
| } | ||
| else { | ||
| if (REALM_UNLIKELY(m_deactivation_initiated) || REALM_UNLIKELY(m_error_message_received)) { | ||
| // Deactivation has been initiated. If the UNBIND message has not been | ||
| // sent yet, there is no point in sending it. Instead, we can let the | ||
| // deactivation process complete. | ||
| if (!m_bind_message_sent) { |
There was a problem hiding this comment.
As a slight change from the old code, we will evaluate this condition when m_deactivation_initiated is false, and m_error_message_received is true. However, there is no behavior change since if m_error_message_received is true, m_bind_message_sent must also be true, so we never enter this if block. See
realm-core/src/realm/sync/noinst/client_impl_base.cpp
Lines 2524 to 2546 in 53ffc48
| REALM_ASSERT(m_upload_progress.client_version <= m_upload_target_version); | ||
| REALM_ASSERT(m_upload_target_version <= m_last_version_available); | ||
| bool need_upload = (m_upload_target_version > m_upload_progress.client_version); | ||
| if (REALM_UNLIKELY(need_upload) && REALM_LIKELY(m_allow_upload)) |
There was a problem hiding this comment.
Do we think that all these LIKELY hints are helping us? At the very least, I don't think we can know whether this is likely or unlikely and would be included to remove this hint.
| } | ||
| } | ||
| else { | ||
| if (REALM_UNLIKELY(m_deactivation_initiated) || REALM_UNLIKELY(m_error_message_received)) { | ||
| // Deactivation has been initiated. If the UNBIND message has not been | ||
| // sent yet, there is no point in sending it. Instead, we can let the | ||
| // deactivation process complete. | ||
| if (!m_bind_message_sent) { |
…4908) This should be identical to the original version, just easier to read.
This should be identical to the original version, just easier to read.
What, How & Why?
☑️ ToDos
[ ] 📝 Changelog update[ ] 🚦 Tests (or not relevant)