-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Upward Message Passing implementation #1885
Conversation
This commit addresses the UMP part of #1869
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.
Looks good to me! I haven't done a detailed review of ump.rs
, but at a quick read-through I didn't see any problems there.
Co-authored-by: Bernhard Schuster <bernhard@ahoi.io>
// | ||
// let's select 0 as the starting index as a safe bet. | ||
debug_assert!(false); | ||
0 |
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.
Is there a case this could be called with zero dispatchables and this hence being oob?
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.
nah, cur_idx doesn't have to point on an existing element, we always access the underlying vector with .get
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.
Just saying, using an Option<usize>
might not be entirely silly in the long run.
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.
Try to imagine how would it look like in the current version. We would have to add a couple of maps (or god forbid expects). I am not sure that it worth it presently. That said, I don't disagree that it might be more appropriate in the future
} | ||
} | ||
|
||
#[cfg(test)] |
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.
Probably a good idea to split into two files here.
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.
while I usually prefer doing splitting, I don't think this is a good case for that. First, we would have either ump_tests.rs
(since the /router
hosts a bunch of different modules already) or another level of hierarchy like router/ump.rs
and router/ump/tests.rs
, and both options seem awkward. Secondly, the tests are still manageable.
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.
I very like the precise documentation in router and the impl gudie 👍 code LGTM as well (no deep dive though).
Co-authored-by: Bernhard Schuster <bernhard@ahoi.io>
bot merge |
Trying merge. |
Closes #1677
The UMP part of and Closes #1702
The UMP part of #1869
Split from #1679