Add the ability to move a Connectable (#382) #434
Conversation
| friend class bus_terminal::BusBreakerViewImpl; | ||
|
|
||
| friend class node_terminal::BusBreakerViewImpl; | ||
| friend class node_terminal::NodeBreakerViewImpl; |
There was a problem hiding this comment.
Not very proud of that... But this is necessary because move methods are not supposed to be in the public API...
There was a problem hiding this comment.
I totally agree: I wonder if the methods are well placed. Looking to the C++ implementation, you can access to the attach/detach methods of the voltage level from the different views. It's probably better to move the code there to avoid these friend declarations.
Could you please ensure it is feasible or not?
There was a problem hiding this comment.
To be discussed: I'm not sure to understand and/or I think we have an issue in C++ implementation: attach & detach methods are in the public API of the VoltageLevel in C++, and not in Java.
| auto it = std::find_if(m_terminals.begin(), m_terminals.end(), [&oldTerminal](const std::unique_ptr<Terminal>& term) { | ||
| return stdcxx::areSame(*term, oldTerminal); | ||
| }); | ||
| *it = std::move(terminal); |
There was a problem hiding this comment.
Is it working properly if the terminal is not found (could it really happen)?
There was a problem hiding this comment.
Yes, it would crash... But I don't see how it could be possible, because the Connectable has a list of Terminal, and oldTerminal is one of them (by definition).
Maybe we should check it and throw an AssertionError if the terminal could not be found?
There was a problem hiding this comment.
TODO: write a unit test with a detached connectable
| friend class bus_terminal::BusBreakerViewImpl; | ||
|
|
||
| friend class node_terminal::BusBreakerViewImpl; | ||
| friend class node_terminal::NodeBreakerViewImpl; |
There was a problem hiding this comment.
I totally agree: I wonder if the methods are well placed. Looking to the C++ implementation, you can access to the attach/detach methods of the voltage level from the different views. It's probably better to move the code there to avoid these friend declarations.
Could you please ensure it is feasible or not?
Signed-off-by: Sébastien LAIGRE <slaigre@silicom.fr>
Signed-off-by: Sébastien LAIGRE <slaigre@silicom.fr>
…no notification) Signed-off-by: Sébastien LAIGRE <slaigre@silicom.fr>
sebalaig
force-pushed
the
382-move-connectable
branch
from
9108de8
to
7b9e2ef
Compare
sebalaig
added
PR: waiting-for-info-reviewer
and removed
PR: waiting-for-correction
labels
|
Kudos, SonarCloud Quality Gate passed! |
|
I have to dig into this PR: could you please remember me what information you are waiting for? |
Please check if the PR fulfills these requirements (please use
'[x]'to check the checkboxes, or submit the PR and then click the checkboxes)Does this PR already have an issue describing the problem ? If so, link to this issue using
'#XXX'and skip the restCloses #382
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
What is the current behavior? (You can also link to an open issue here)
What is the new behavior (if this is a feature change)?
Does this PR introduce a breaking change or deprecate an API? If yes, check the following:
Other information:
(if any of the questions/checkboxes don't apply, please delete them entirely)