mw/com: Make ProxyBase::AreBindingsValid symmetrical with Skeleton side#414
Open
bemerybmw wants to merge 2 commits into
Open
mw/com: Make ProxyBase::AreBindingsValid symmetrical with Skeleton side#414bemerybmw wants to merge 2 commits into
bemerybmw wants to merge 2 commits into
Conversation
bemerybmw
requested review from
LittleHuba,
castler,
crimson11,
hoe-jo and
limdor
as code owners
Contributor
|
The removal of EventBindingRegistrationGuard would happen later? |
Contributor
Author
Yep, that's happening in the next change that I'm working on. |
KrishaDeshkool
approved these changes
May 13, 2026
Previously, ProxyBase::AreBindingsValid was implemented differently to avoid have to store a map of service elements (which also requires updating the map when moving a Proxy) in ProxyBase. However, we have since added these maps so there's no longer any reason to have the asymmetry in the implementations of AreBindingsValid.
Previously, the event binding was registered from the binding independent level via a function in the bindings interface. We change this so that the registration is done completely on the binding level to simplify the interface and also to allow each binding to decide whether it needs to register events with the parent proxy. This is now inline with how we handle registration for methods. This also fixes an issue that the EventRegistrationGuard was accessing the Proxy binding from the context of a ProxyEvent's destructor. When move assigning a skeleton, the ProxyBase move assignment operator will be called before that of the ProxyEvent, so the pointer to the Proxy binding would be invalid when calling UnregisterEventBinding.
bemerybmw
force-pushed
the
brem_fix_event_registration_impl
branch
from
fe22c01 to
d496d88
Compare
crimson11
requested changes
May 13, 2026
| Instead, they only set once (see above) the `ProxyBase::are_service_element_bindings_valid_` member variable of their | ||
| parent during construction as this is currently the only feedback needed between proxy and its proxy events on binding | ||
| independent level. | ||
| Similarly to the skeleton side, the `impl::SkeletonBase` doesn't "know" its event/field/method children. Therefore, |
Contributor
There was a problem hiding this comment.
impl::SkeletonBase
impl::ProxyBase
|
|
||
| #### Binding level Registration of proxy events/fields at their parent proxy | ||
|
|
||
| On the binding **specific** level things look different! |
Contributor
There was a problem hiding this comment.
Does this sentence now make sense anymore? So there is no difference between binding-specific/binding-unspecific level anymore? In both cases the proxy knows its childs, because on both levels this registration process takes place ...
|
|
||
| // Expecting that StartFindService is called and synchronously calls handler since provider exists | ||
| EXPECT_CALL(service_discovery_mock_, StartFindService(_, EnrichedInstanceIdentifier{identifier_})) | ||
| .WillOnce(WithArg<0>(Invoke([valid_find_service_handle, this](auto find_service_handler) noexcept { |
Contributor
There was a problem hiding this comment.
Nitpicky:
.WillOnce(InvokeArgument<0>([valid_find_service_handle, this](auto find_service_handler) noexcept {
is a lot clearer ;)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Previously, ProxyBase::AreBindingsValid was implemented differently to avoid have to store a map of service elements (which also requires updating the map when moving a Proxy) in ProxyBase. However, we have since added these maps so there's no longer any reason to have the asymmetry in the implementations of AreBindingsValid.