-
Notifications
You must be signed in to change notification settings - Fork 388
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
iox-#938 check for publisher destruction in does violate communication policy #1131
iox-#938 check for publisher destruction in does violate communication policy #1131
Conversation
@@ -38,7 +46,7 @@ PortManager::doesViolateCommunicationPolicy(const capro::ServiceDescription& ser | |||
|
|||
template <typename T, std::enable_if_t<std::is_same<T, iox::build::ManyToManyPolicy>::value>*> | |||
inline cxx::optional<RuntimeName_t> | |||
PortManager::doesViolateCommunicationPolicy(const capro::ServiceDescription& service IOX_MAYBE_UNUSED) const noexcept | |||
PortManager::doesViolateCommunicationPolicy(const capro::ServiceDescription& service IOX_MAYBE_UNUSED) noexcept |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PortManager::doesViolateCommunicationPolicy(const capro::ServiceDescription& service IOX_MAYBE_UNUSED) noexcept | |
PortManager::doesViolateCommunicationPolicy(const capro::ServiceDescription&) noexcept |
Codecov Report
@@ Coverage Diff @@
## master #1131 +/- ##
==========================================
+ Coverage 77.23% 77.27% +0.03%
==========================================
Files 346 346
Lines 13159 13159
Branches 1884 1884
==========================================
+ Hits 10164 10168 +4
+ Misses 2371 2369 -2
+ Partials 624 622 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
if (publisherPortData->m_toBeDestroyed) | ||
{ | ||
destroyPublisherPort(publisherPortData); | ||
continue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, we ignore those that are about to be destroyed in the violation check (i.e. whether there already is such a port).
Now it might be possible that a port is about to be marked for destruction but not quite (i.e. m_toBeDestroyed == false
) but we deny another port to be constructed. This is ok I guess, since we could argue that at this point the port still lives (and hence we cannot construct another one).
If so, the logic is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MatthiasKillat correct, we can only reason about what we know and at this point only the ones who already have the flag set are not alive animore
3bf9bd9
81011e7
to
3bf9bd9
Compare
3bf9bd9
to
43362df
Compare
Pre-Review Checklist for the PR Author
iox-#123-this-is-a-branch
)iox-#123 commit text
)git commit -s
)task-list-completed
)Notes for Reviewer
This PR checks whether the publisher port which would cause a communication policy violation is marked for destruction and destroys it before asserting the error.
Checklist for the PR Reviewer
Post-review Checklist for the PR Author
References