Skip to content

feat(meta): LazyBin & LazyObject#149

Open
alanpq wants to merge 2 commits into
mainfrom
feat/bin-streaming
Open

feat(meta): LazyBin & LazyObject#149
alanpq wants to merge 2 commits into
mainfrom
feat/bin-streaming

Conversation

@alanpq

@alanpq alanpq commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

No description provided.

@alanpq alanpq requested a review from Crauzer July 9, 2026 14:48
@alanpq alanpq force-pushed the feat/bin-streaming branch from 66d6689 to 251edbe Compare July 9, 2026 14:49
@alanpq alanpq marked this pull request as draft July 9, 2026 14:49
@alanpq alanpq force-pushed the feat/bin-streaming branch 2 times, most recently from 5574d62 to b817943 Compare July 9, 2026 14:53
@alanpq alanpq force-pushed the feat/bin-streaming branch from b817943 to fdb2b7e Compare July 9, 2026 14:54
@alanpq alanpq force-pushed the feat/bin-streaming branch from fdb2b7e to 854bee4 Compare July 9, 2026 14:54
@alanpq alanpq marked this pull request as ready for review July 9, 2026 14:54
is_override: bool,
objects: Vec<BinObject<M>>,
dependencies: Vec<String>,
dependencies: Vec<PrefixString<u16>>,

@Crauzer Crauzer Jul 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure whether this is worth the change, might be better to just leave it as is imo, we can have a sanity check when writing.
This also breaks the public API for anybody accessing .dependencies

// Always write version 3
writer.write_u32::<LE>(WRITE_VERSION)?;
writer.write_u32::<LE>(self.dependencies.len() as _)?;
writer.write_u32::<LE>(self.version)?;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should keep version hardcoded to 3, don't think we gain anything by not

writer.write_u32::<LE>(WRITE_VERSION)?;
writer.write_u32::<LE>(self.dependencies.len() as _)?;
writer.write_u32::<LE>(self.version)?;
writer.write_u32::<LE>(self.dependencies.len().try_into().unwrap())?;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related to comment above, this breaks the file when you write version 1 e2e for example.

/// ```
pub fn to_writer<W: io::Write + io::Seek + ?Sized>(&self, writer: &mut W) -> io::Result<()> {
match self.is_override {
true => todo!("implement is_override Bin write"),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably leave in the todo! panic as the changes in this PR make the writer output broken files.

if self.is_override {
            writer.write_u32::<LE>(self.data_overrides.len() as _)?;
            // TODO: impl data overrides
            //for o in &self.data_overrides {
            //}
        }

^ not correct since you always write PropHeader::MAGIC

})
}

pub fn object(self, path: impl Into<BinHash>) -> Result<Option<LazyObject<'r, R>>, ReadErr> {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of consuming self here for no reason, we should be able to have it as &mut self.
Right now you need to re-parse the header for each object you want to fetch lazily.
Fixable if we keep a lookup table of all entries offset and size, that way you can easily hop around the stream to the entry you need.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants