4 ms·
Nice read! Nit: // Order the task's regions in ascending address order. // // THIS IS IMPORTANT. The kernel exploits this property to do cheaper
by scottlamb 3y ago
Nice read!
Nit:
// Order the task's regions in ascending address order.
//
// THIS IS IMPORTANT. The kernel exploits this property to do cheaper
// access tests.
regions.sort_by_key(|i| region_table.get_index(*i).unwrap().1.base);
I wouldn't put this comment here. It's not just some detail of this function; it's an invariant of the field that all writers have to respect (maybe this is the only one now but still) and all readers can take advantage of. So I'd add it to the `TaskDesc::regions` docstring. [1]
[1] https://github.com/oxidecomputer/hubris/commit/b44e677fb39cde8be5b10bbf78a9f26c000f6ad6#diff-96e27941c050f6013357d7ad1716058d7171f2ab012e2bc5ee01a6c19701d826L45 https://github.com/oxidecomputer/hubris/commit/b44e677fb39cd...
- db48x 3y agoIt is nice to have the comment next to the sort though, which is otherwise going to be surprising. Probably the best thing to do is to make a constructor method for the TaskDesc that sorts the regions to enforce the invariants. The code is evidently getting more complex over time, so packing the complexity up into methods is probably now worth spending a little time on.