Conversation
This commit addresses the UMP part of #1869
pepyakin
added
A0-please_review
coriolinus
left a comment
coriolinus
left a comment
There was a problem hiding this comment.
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.
Is there a case this could be called with zero dispatchables and this hence being oob?
There was a problem hiding this comment.
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.
Just saying, using an Option<usize> might not be entirely silly in the long run.
There was a problem hiding this comment.
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.
Probably a good idea to split into two files here.
There was a problem hiding this comment.
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.
drahnr
left a comment
drahnr
left a comment
There was a problem hiding this comment.
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. |
ghost
merged commit 4 participants
Closes #1677
The UMP part of and Closes #1702
The UMP part of #1869
Split from #1679