This is an archive of the discontinued Mercurial Phabricator instance.

Differential D9102

hg-core: use `u32` instead of `i32` in `Chunk` (D8958#inline-15001 followup)
ClosedPublic

Authored by acezar on Sep 28 2020, 9:48 AM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Commits
rHGd07e4656ff5a: hg-core: use `u32` instead of `i32` in `Chunk` (D8958#inline-15001 followup)

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

acezar created this revision.Sep 28 2020, 9:48 AM
Alphare added a subscriber: Alphare.Sep 28 2020, 10:22 AM
Alphare added inline comments.
rust/hg-core/src/revlog/patch.rs
25–28

Is the offset always positive? Same question for all affected methods.

acezar marked an inline comment as done.Sep 29 2020, 4:32 AM
acezar added inline comments.
rust/hg-core/src/revlog/patch.rs
25–28

The start of the replaced chunk cannot be negative. Negative start would mean that the replacement start before the start of the data. Same for the other methods.

Alphare added inline comments.Sep 29 2020, 4:35 AM
rust/hg-core/src/revlog/patch.rs
25–28

I'm only asking because if self.start is 0, and offset is -1, the as u32 conversion would put the result to u32::max_value(). But maybe that never happens?

acezar marked an inline comment as done.Sep 29 2020, 4:40 AM
acezar added inline comments.
rust/hg-core/src/revlog/patch.rs
25–28

Theoretically no. But if there is a mistake... maybe checked_add should be used

acezar updated this revision to Diff 22912.Sep 29 2020, 4:57 AM
martinvonz added a subscriber: martinvonz.Sep 29 2020, 1:42 PM
martinvonz added inline comments.
rust/hg-core/src/revlog/patch.rs
114

nit: delta seems like a clearer name

Antoine cezar <acezar@chwitlabs.fr> added a commit: rHGd07e4656ff5a: hg-core: use `u32` instead of `i32` in `Chunk` (D8958#inline-15001 followup).Sep 29 2020, 1:52 PM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHGd07e4656ff5a: hg-core: use `u32` instead of `i32` in `Chunk` (D8958#inline-15001 followup) (authored by Antoine cezar <acezar@chwitlabs.fr>). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mrust/hg-core/src/revlog/patch.rs (32 lines)

Diff 22912

rust/hg-core/src/revlog/patch.rs

use byteorder::{BigEndian, ByteOrder}; use byteorder::{BigEndian, ByteOrder};
/// A chunk of data to insert, delete or replace in a patch /// A chunk of data to insert, delete or replace in a patch
/// ///
/// A chunk is: /// A chunk is:
/// - an insertion when `!data.is_empty() && start == end` /// - an insertion when `!data.is_empty() && start == end`
/// - an deletion when `data.is_empty() && start < end` /// - an deletion when `data.is_empty() && start < end`
/// - a replacement when `!data.is_empty() && start < end` /// - a replacement when `!data.is_empty() && start < end`
/// - not doing anything when `data.is_empty() && start == end` /// - not doing anything when `data.is_empty() && start == end`
#[derive(Debug, Clone)] #[derive(Debug, Clone)]
struct Chunk<'a> { struct Chunk<'a> {
/// The start position of the chunk of data to replace /// The start position of the chunk of data to replace
start: i32, start: u32,
/// The end position of the chunk of data to replace (open end interval) /// The end position of the chunk of data to replace (open end interval)
end: i32, end: u32,
/// The data replacing the chunk /// The data replacing the chunk
data: &'a [u8], data: &'a [u8],
} }
impl<'a> Chunk<'a> { impl<'a> Chunk<'a> {
/// Adjusted start of the chunk to replace. /// Adjusted start of the chunk to replace.
/// ///
/// Offset allow to take into account the growth/shrinkage of data /// Offset allow to take into account the growth/shrinkage of data
/// induced by previously applied chunks. /// induced by previously applied chunks.
fn start_offseted_by(&self, offset: i32) -> i32 { fn start_offseted_by(&self, offset: i32) -> u32 {
self.start + offset let start = self.start as i32 + offset;
assert!(start >= 0, "negative chunk start should never happen");
start as u32
AlphareUnsubmitted
Done

Is the offset always positive? Same question for all affected methods.

Alphare: Is the offset always positive? Same question for all affected methods.
acezarAuthorUnsubmitted
Done

The start of the replaced chunk cannot be negative. Negative start would mean that the replacement start before the start of the data. Same for the other methods.

acezar: The start of the replaced chunk cannot be negative. Negative start would mean that the…
AlphareUnsubmitted
Not Done

I'm only asking because if self.start is 0, and offset is -1, the as u32 conversion would put the result to u32::max_value(). But maybe that never happens?

Alphare: I'm only asking because if `self.start` is `0`, and `offset` is `-1`, the `as u32` conversion…
acezarAuthorUnsubmitted
Done

Theoretically no. But if there is a mistake... maybe checked_add should be used

acezar: Theoretically no. But if there is a mistake... maybe `checked_add` should be used
} }
/// Adjusted end of the chunk to replace. /// Adjusted end of the chunk to replace.
/// ///
/// Offset allow to take into account the growth/shrinkage of data /// Offset allow to take into account the growth/shrinkage of data
/// induced by previously applied chunks. /// induced by previously applied chunks.
fn end_offseted_by(&self, offset: i32) -> i32 { fn end_offseted_by(&self, offset: i32) -> u32 {
self.start_offseted_by(offset) + (self.data.len() as i32) self.start_offseted_by(offset) + self.data.len() as u32
} }
/// Length of the replaced chunk. /// Length of the replaced chunk.
fn replaced_len(&self) -> i32 { fn replaced_len(&self) -> u32 {
self.end - self.start self.end - self.start
} }
/// Length difference between the replacing data and the replaced data. /// Length difference between the replacing data and the replaced data.
fn len_diff(&self) -> i32 { fn len_diff(&self) -> i32 {
(self.data.len() as i32) - self.replaced_len() self.data.len() as i32 - self.replaced_len() as i32
} }
} }
/// The delta between two revisions data. /// The delta between two revisions data.
#[derive(Debug, Clone)] #[derive(Debug, Clone)]
pub struct PatchList<'a> { pub struct PatchList<'a> {
/// A collection of chunks to apply. /// A collection of chunks to apply.
/// ///
/// Those chunks are: /// Those chunks are:
/// - ordered from the left-most replacement to the right-most replacement /// - ordered from the left-most replacement to the right-most replacement
/// - non-overlapping, meaning that two chucks can not change the same /// - non-overlapping, meaning that two chucks can not change the same
/// chunk of the patched data /// chunk of the patched data
chunks: Vec<Chunk<'a>>, chunks: Vec<Chunk<'a>>,
} }
impl<'a> PatchList<'a> { impl<'a> PatchList<'a> {
/// Create a `PatchList` from bytes. /// Create a `PatchList` from bytes.
pub fn new(data: &'a [u8]) -> Self { pub fn new(data: &'a [u8]) -> Self {
let mut chunks = vec![]; let mut chunks = vec![];
let mut data = data; let mut data = data;
while !data.is_empty() { while !data.is_empty() {
let start = BigEndian::read_i32(&data[0..]); let start = BigEndian::read_u32(&data[0..]);
let end = BigEndian::read_i32(&data[4..]); let end = BigEndian::read_u32(&data[4..]);
let len = BigEndian::read_i32(&data[8..]); let len = BigEndian::read_u32(&data[8..]);
assert!(0 <= start && start <= end && len >= 0); assert!(start <= end);
chunks.push(Chunk { chunks.push(Chunk {
start, start,
end, end,
data: &data[12..12 + (len as usize)], data: &data[12..12 + (len as usize)],
}); });
data = &data[12 + (len as usize)..]; data = &data[12 + (len as usize)..];
} }
PatchList { chunks } PatchList { chunks }
/// Applying consecutive patches can lead to waste of time and memory /// Applying consecutive patches can lead to waste of time and memory
/// as the changes introduced by one patch can be overridden by the next. /// as the changes introduced by one patch can be overridden by the next.
/// Combining patches optimizes the whole patching sequence. /// Combining patches optimizes the whole patching sequence.
fn combine(&mut self, other: &mut Self) -> Self { fn combine(&mut self, other: &mut Self) -> Self {
let mut chunks = vec![]; let mut chunks = vec![];
// Keep track of each growth/shrinkage resulting from applying a chunk // Keep track of each growth/shrinkage resulting from applying a chunk
// in order to adjust the start/end of subsequent chunks. // in order to adjust the start/end of subsequent chunks.
let mut offset = 0i32; let mut offset = 0i32;
martinvonzUnsubmitted
Not Done

nit: delta seems like a clearer name

martinvonz: nit: `delta` seems like a clearer name
// Keep track of the chunk of self.chunks to process. // Keep track of the chunk of self.chunks to process.
let mut pos = 0; let mut pos = 0;
// For each chunk of `other`, chunks of `self` are processed // For each chunk of `other`, chunks of `self` are processed
// until they start after the end of the current chunk. // until they start after the end of the current chunk.
for Chunk { start, end, data } in other.chunks.iter() { for Chunk { start, end, data } in other.chunks.iter() {
// Add chunks of `self` that start before this chunk of `other` // Add chunks of `self` that start before this chunk of `other`
{ {
let first = &mut self.chunks[pos]; let first = &mut self.chunks[pos];
let how_much_to_discard = let how_much_to_discard =
*end - first.start_offseted_by(next_offset); *end - first.start_offseted_by(next_offset);
first.data = &first.data[(how_much_to_discard as usize)..]; first.data = &first.data[(how_much_to_discard as usize)..];
next_offset += how_much_to_discard; next_offset += how_much_to_discard as i32;
} }
// Add the chunk of `other` with adjusted position. // Add the chunk of `other` with adjusted position.
chunks.push(Chunk { chunks.push(Chunk {
start: *start - offset, start: (*start as i32 - offset) as u32,
end: *end - next_offset, end: (*end as i32 - next_offset) as u32,
data, data,
}); });
// Go back to normal offset tracking for the next `o` chunk // Go back to normal offset tracking for the next `o` chunk
offset = next_offset; offset = next_offset;
} }
// Add remaining chunks of `self`. // Add remaining chunks of `self`.