From c9b9e2864eeae82efdf169656ed1c01eac996b39 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Thu, 21 Dec 2023 17:47:19 +0900 Subject: [PATCH] index: introduce newtype that represents segment-local position I'm thinking of changing some IndexSegment methods to return LocalPosition instead of global IndexPosition, and using u32 there would be a source of bugs. --- lib/src/default_index/composite.rs | 16 +++++++++------- lib/src/default_index/entry.rs | 15 ++++++++++++--- lib/src/default_index/mutable.rs | 22 +++++++++++----------- lib/src/default_index/readonly.rs | 18 +++++++++--------- 4 files changed, 41 insertions(+), 30 deletions(-) diff --git a/lib/src/default_index/composite.rs b/lib/src/default_index/composite.rs index 5358823a4..c7464a6de 100644 --- a/lib/src/default_index/composite.rs +++ b/lib/src/default_index/composite.rs @@ -21,7 +21,9 @@ use std::sync::Arc; use itertools::Itertools; -use super::entry::{IndexEntry, IndexPosition, IndexPositionByGeneration, SmallIndexPositionsVec}; +use super::entry::{ + IndexEntry, IndexPosition, IndexPositionByGeneration, LocalPosition, SmallIndexPositionsVec, +}; use super::readonly::ReadonlyIndexSegment; use super::rev_walk::RevWalk; use crate::backend::{ChangeId, CommitId, ObjectId}; @@ -50,15 +52,15 @@ pub(super) trait IndexSegment: Send + Sync { fn segment_resolve_prefix(&self, prefix: &HexPrefix) -> PrefixResolution; - fn segment_generation_number(&self, local_pos: u32) -> u32; + fn segment_generation_number(&self, local_pos: LocalPosition) -> u32; - fn segment_commit_id(&self, local_pos: u32) -> CommitId; + fn segment_commit_id(&self, local_pos: LocalPosition) -> CommitId; - fn segment_change_id(&self, local_pos: u32) -> ChangeId; + fn segment_change_id(&self, local_pos: LocalPosition) -> ChangeId; - fn segment_num_parents(&self, local_pos: u32) -> u32; + fn segment_num_parents(&self, local_pos: LocalPosition) -> u32; - fn segment_parent_positions(&self, local_pos: u32) -> SmallIndexPositionsVec; + fn segment_parent_positions(&self, local_pos: LocalPosition) -> SmallIndexPositionsVec; } /// Abstraction over owned and borrowed types that can be cheaply converted to @@ -151,7 +153,7 @@ impl<'a> CompositeIndex<'a> { self.ancestor_index_segments() .find_map(|segment| { u32::checked_sub(pos.0, segment.segment_num_parent_commits()) - .map(|local_pos| IndexEntry::new(segment, pos, local_pos)) + .map(|local_pos| IndexEntry::new(segment, pos, LocalPosition(local_pos))) }) .unwrap() } diff --git a/lib/src/default_index/entry.rs b/lib/src/default_index/entry.rs index d741da06e..626511e78 100644 --- a/lib/src/default_index/entry.rs +++ b/lib/src/default_index/entry.rs @@ -22,6 +22,7 @@ use smallvec::SmallVec; use super::composite::{CompositeIndex, IndexSegment}; use crate::backend::{ChangeId, CommitId, ObjectId}; +/// Global index position. #[derive(Debug, PartialEq, Eq, PartialOrd, Ord, Clone, Copy, Hash)] pub struct IndexPosition(pub(super) u32); @@ -29,6 +30,10 @@ impl IndexPosition { pub const MAX: Self = IndexPosition(u32::MAX); } +/// Local position within an index segment. +#[derive(Debug, PartialEq, Eq, PartialOrd, Ord, Clone, Copy, Hash)] +pub(super) struct LocalPosition(pub(super) u32); + // SmallVec reuses two pointer-size fields as inline area, which meas we can // inline up to 16 bytes (on 64-bit platform) for free. pub(super) type SmallIndexPositionsVec = SmallVec<[IndexPosition; 4]>; @@ -37,8 +42,8 @@ pub(super) type SmallIndexPositionsVec = SmallVec<[IndexPosition; 4]>; pub struct IndexEntry<'a> { source: &'a dyn IndexSegment, pos: IndexPosition, - // Position within the source segment - local_pos: u32, + /// Position within the source segment + local_pos: LocalPosition, } impl Debug for IndexEntry<'_> { @@ -66,7 +71,11 @@ impl Hash for IndexEntry<'_> { } impl<'a> IndexEntry<'a> { - pub(super) fn new(source: &'a dyn IndexSegment, pos: IndexPosition, local_pos: u32) -> Self { + pub(super) fn new( + source: &'a dyn IndexSegment, + pos: IndexPosition, + local_pos: LocalPosition, + ) -> Self { IndexEntry { source, pos, diff --git a/lib/src/default_index/mutable.rs b/lib/src/default_index/mutable.rs index 6b9914b24..a749bfa5f 100644 --- a/lib/src/default_index/mutable.rs +++ b/lib/src/default_index/mutable.rs @@ -30,7 +30,7 @@ use smallvec::SmallVec; use tempfile::NamedTempFile; use super::composite::{AsCompositeIndex, CompositeIndex, IndexSegment}; -use super::entry::{IndexPosition, SmallIndexPositionsVec}; +use super::entry::{IndexPosition, LocalPosition, SmallIndexPositionsVec}; use super::readonly::{DefaultReadonlyIndex, ReadonlyIndexSegment}; use crate::backend::{ChangeId, CommitId, ObjectId}; use crate::commit::Commit; @@ -351,28 +351,28 @@ impl IndexSegment for MutableIndexSegment { } } - fn segment_generation_number(&self, local_pos: u32) -> u32 { - self.graph[local_pos as usize].generation_number + fn segment_generation_number(&self, local_pos: LocalPosition) -> u32 { + self.graph[local_pos.0 as usize].generation_number } - fn segment_commit_id(&self, local_pos: u32) -> CommitId { - self.graph[local_pos as usize].commit_id.clone() + fn segment_commit_id(&self, local_pos: LocalPosition) -> CommitId { + self.graph[local_pos.0 as usize].commit_id.clone() } - fn segment_change_id(&self, local_pos: u32) -> ChangeId { - self.graph[local_pos as usize].change_id.clone() + fn segment_change_id(&self, local_pos: LocalPosition) -> ChangeId { + self.graph[local_pos.0 as usize].change_id.clone() } - fn segment_num_parents(&self, local_pos: u32) -> u32 { - self.graph[local_pos as usize] + fn segment_num_parents(&self, local_pos: LocalPosition) -> u32 { + self.graph[local_pos.0 as usize] .parent_positions .len() .try_into() .unwrap() } - fn segment_parent_positions(&self, local_pos: u32) -> SmallIndexPositionsVec { - self.graph[local_pos as usize].parent_positions.clone() + fn segment_parent_positions(&self, local_pos: LocalPosition) -> SmallIndexPositionsVec { + self.graph[local_pos.0 as usize].parent_positions.clone() } } diff --git a/lib/src/default_index/readonly.rs b/lib/src/default_index/readonly.rs index 9181f8276..ef1f18d4e 100644 --- a/lib/src/default_index/readonly.rs +++ b/lib/src/default_index/readonly.rs @@ -27,7 +27,7 @@ use smallvec::SmallVec; use thiserror::Error; use super::composite::{AsCompositeIndex, CompositeIndex, IndexSegment}; -use super::entry::{IndexPosition, SmallIndexPositionsVec}; +use super::entry::{IndexPosition, LocalPosition, SmallIndexPositionsVec}; use super::mutable::DefaultMutableIndex; use crate::backend::{ChangeId, CommitId, ObjectId}; use crate::default_revset_engine; @@ -310,9 +310,9 @@ impl ReadonlyIndexSegment { self.change_id_length } - fn graph_entry(&self, local_pos: u32) -> CommitGraphEntry { - assert!(local_pos < self.num_local_commits); - let offset = (local_pos as usize) * self.commit_graph_entry_size; + fn graph_entry(&self, local_pos: LocalPosition) -> CommitGraphEntry { + assert!(local_pos.0 < self.num_local_commits); + let offset = (local_pos.0 as usize) * self.commit_graph_entry_size; CommitGraphEntry { data: &self.data[offset..][..self.commit_graph_entry_size], commit_id_length: self.commit_id_length, @@ -426,23 +426,23 @@ impl IndexSegment for ReadonlyIndexSegment { } } - fn segment_generation_number(&self, local_pos: u32) -> u32 { + fn segment_generation_number(&self, local_pos: LocalPosition) -> u32 { self.graph_entry(local_pos).generation_number() } - fn segment_commit_id(&self, local_pos: u32) -> CommitId { + fn segment_commit_id(&self, local_pos: LocalPosition) -> CommitId { self.graph_entry(local_pos).commit_id() } - fn segment_change_id(&self, local_pos: u32) -> ChangeId { + fn segment_change_id(&self, local_pos: LocalPosition) -> ChangeId { self.graph_entry(local_pos).change_id() } - fn segment_num_parents(&self, local_pos: u32) -> u32 { + fn segment_num_parents(&self, local_pos: LocalPosition) -> u32 { self.graph_entry(local_pos).num_parents() } - fn segment_parent_positions(&self, local_pos: u32) -> SmallIndexPositionsVec { + fn segment_parent_positions(&self, local_pos: LocalPosition) -> SmallIndexPositionsVec { let graph_entry = self.graph_entry(local_pos); let mut parent_entries = SmallVec::with_capacity(graph_entry.num_parents() as usize); if graph_entry.num_parents() >= 1 {