Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(res): support 16-bit entry offsets #2269

Merged
merged 1 commit into from
Sep 6, 2024

Conversation

pubiqq
Copy link
Contributor

@pubiqq pubiqq commented Sep 6, 2024

Fixes #2267

@skylot
Copy link
Owner

skylot commented Sep 6, 2024

Hm, with sample provided in #2267 (comment) parsing fail at closest

is.checkPos(entriesStart, "Expected first entry start");

because of additional 2 bytes, maybe it is padding, so it might be better to change this line to

is.skipToPos(entriesStart, "Failed to skip to entries start");

And, as soon as, sample also use compact entries, it still can't be parsed, so I am not sure if this is a correct solution.
To be honest, it is better to merge this PR with compact entries support, because this is the only way to verify its correctness.

@pubiqq
Copy link
Contributor Author

pubiqq commented Sep 6, 2024

This PR only adds support for 16-bit entry offsets. Not support for compact entries, not support for non-standard paddings, only 16-bit entry offsets.

But if you insist on implementing all together, then you can close the PR. I don't think I'll create a PR with compact entries support, because my current implementation of the resource parser is too different from yours.

@skylot
Copy link
Owner

skylot commented Sep 6, 2024

I see.
I will try to implement compact entries support myself and will merge this PR once I managed to parse provided sample.

@skylot skylot self-assigned this Sep 6, 2024
@skylot skylot merged commit 937dd20 into skylot:master Sep 6, 2024
5 checks passed
@pubiqq pubiqq deleted the support-offset16 branch October 10, 2024 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[core] 16-bit resource entry offsets are not supported
2 participants