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

anlz: Use binrw instead of nom to parse ANLZ*.DAT files #47

Merged
merged 5 commits into from
Feb 25, 2022
Merged

Conversation

Holzhaus
Copy link
Owner

Closes #46.

src/anlz.rs Outdated Show resolved Hide resolved
src/bin/rekordcrate-anlz.rs Outdated Show resolved Hide resolved
src/util.rs Outdated Show resolved Hide resolved
@Holzhaus Holzhaus marked this pull request as ready for review February 25, 2022 10:20
@Holzhaus Holzhaus added this to the 0.1.1 milestone Feb 25, 2022
src/anlz.rs Outdated
Comment on lines 96 to 98
/// Unknown Kind.
Unknown([u8; 4]),
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a short explanation for each Unknown why that couldn't be removed?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Copy link
Owner Author

Choose a reason for hiding this comment

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

Without this, we wouldn't be able to parse *.2EX files. See #9.

Comment on lines +600 to +602
/// Beats in this beatgrid.
#[br(count = len_beats)]
pub beats: Vec<Beat>,
Copy link
Contributor

Choose a reason for hiding this comment

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

note that exposing the Vec publicly mutably means that the beats.len() == len_beats invariant can be violated. binrw currently does not have a way to fix these up easily. see the issue where this is discussed in the context of binrw::FilePtr: jam1garner/binrw#4

Copy link
Owner Author

Choose a reason for hiding this comment

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

note that exposing the Vec publicly mutably means that the beats.len() == len_beats invariant can be violated.

Isn't that the case anyway even if the field was not pub? I could still crate an instance of the struct with mismatching len_beats and beats values by hand. The fix would be to make len_beats a temp field that isn't actually part of the struct and is only parsed for reading and automatically calculated from beats when writing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure, as mentioned in the FilePtr issue, a second pass could probably help with most of these issues. Another one could be to pass a reference to the field in the types args, but that is not supported either currently. Though both are on the v0.9.0 roadmap https://github.com/users/jam1garner/projects/1

Isn't that the case anyway even if the field was not pub? I could still crate an instance of the struct with mismatching len_beats and beats values by hand.

well if the type is public then yes, for this reason, I opted for an encapsulation approach in the DeviceSQLString implementation where the string is constructed correctly once and then should never be manipulated again, at least thats the plan, there are still some kinks to work out.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Should be fixed with #51.

@Holzhaus Holzhaus merged commit 7263838 into main Feb 25, 2022
@Holzhaus Holzhaus deleted the anlz-binrw branch October 6, 2022 16:29
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.

Port anlz.rs from nom to binrw
2 participants