From 863d04a8ce931e9e3bafde5d7dd4b3bdd2fb178b Mon Sep 17 00:00:00 2001 From: Kerstin Humm Date: Sun, 12 Nov 2023 15:04:06 +0100 Subject: [PATCH 1/4] (refactor): update binrw to 0.13 --- Cargo.lock | 8 ++-- Cargo.toml | 2 +- build.rs | 2 +- src/anlz.rs | 18 ++++---- src/main.rs | 7 ++-- src/pdb/mod.rs | 95 ++++++++++++++++++++++--------------------- src/setting.rs | 6 +-- src/util.rs | 29 ++++++------- tests/tests_pdb.rs.in | 2 +- 9 files changed, 85 insertions(+), 84 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 0a54eaea..6dd5335c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -68,9 +68,9 @@ checksum = "3d62b7694a562cdf5a74227903507c56ab2cc8bdd1f781ed5cb4cf9c9f810bfc" [[package]] name = "binrw" -version = "0.10.0" +version = "0.13.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f846d8732b2a55b569b885852ecc925a2b1f24568f4707f8b1ccd5dc6805ea9b" +checksum = "5a1b8720bedc0a503fd5c90bef3fbdc397144ac7efcc5610b30bde08083d7495" dependencies = [ "array-init", "binrw_derive", @@ -79,9 +79,9 @@ dependencies = [ [[package]] name = "binrw_derive" -version = "0.10.0" +version = "0.13.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5c2aa66a5e35daf7f91ed44c945886597ef4c327f34f68b6bbf22951a250ceeb" +checksum = "0741a1b1a70a36a1e7fb0316bba0828f6487a3b6e577353cf974b59fbbb92081" dependencies = [ "either", "owo-colors", diff --git a/Cargo.toml b/Cargo.toml index 0ab2ba77..1d46a582 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -12,7 +12,7 @@ edition = "2021" exclude = [".*"] [dependencies] -binrw = "0.10" +binrw = "0.13" modular-bitfield = "0.11" crc16 = "0.4" clap = { version = "4.0.10", features = ["derive"], optional = true } diff --git a/build.rs b/build.rs index e516fefa..25561812 100644 --- a/build.rs +++ b/build.rs @@ -53,7 +53,7 @@ fn main() { out_dir.join("tests_pdb.rs"), "data/complete_export/*/PIONEER/rekordbox/export.pdb", r#"// THIS FILE IS AUTOGENERATED - DO NOT EDIT! -use binrw::{{BinRead, ReadOptions}}; +use binrw::{{BinRead}}; use rekordcrate::pdb::Header; "#, "./tests/tests_pdb.rs.in" diff --git a/src/anlz.rs b/src/anlz.rs index f31a3091..db28d8d4 100644 --- a/src/anlz.rs +++ b/src/anlz.rs @@ -30,7 +30,7 @@ use crate::{util::ColorIndex, xor::XorStream}; use binrw::{ binrw, io::{Read, Seek, Write}, - BinRead, BinResult, BinWrite, NullWideString, ReadOptions, WriteOptions, + BinRead, BinResult, BinWrite, Endian, NullWideString, }; use modular_bitfield::prelude::*; @@ -867,15 +867,15 @@ impl SongStructureData { /// value. fn read_encrypted( reader: &mut R, - options: &ReadOptions, + endian: Endian, (is_encrypted, len_entries): (bool, u16), ) -> BinResult { if is_encrypted { let key: Vec = Self::get_key(len_entries).collect(); let mut xor_reader = XorStream::with_key(reader, key); - Self::read_options(&mut xor_reader, options, (len_entries,)) + Self::read_options(&mut xor_reader, endian, (len_entries,)) } else { - Self::read_options(reader, options, (len_entries,)) + Self::read_options(reader, endian, (len_entries,)) } } @@ -884,15 +884,15 @@ impl SongStructureData { fn write_encrypted( &self, writer: &mut W, - options: &WriteOptions, + endian: Endian, (is_encrypted, len_entries): (bool, u16), ) -> BinResult<()> { if is_encrypted { let key: Vec = Self::get_key(len_entries).collect(); let mut xor_writer = XorStream::with_key(writer, key); - self.write_options(&mut xor_writer, options, ()) + self.write_options(&mut xor_writer, endian, ()) } else { - self.write_options(writer, options, ()) + self.write_options(writer, endian, ()) } } } @@ -943,7 +943,7 @@ pub struct ANLZ { impl ANLZ { fn parse_sections( reader: &mut R, - ro: &ReadOptions, + endian: Endian, args: (u32,), ) -> BinResult> { let (content_size,) = args; @@ -951,7 +951,7 @@ impl ANLZ { let mut sections: Vec
= vec![]; while reader.stream_position()? < final_position { - let section = Section::read_options(reader, ro, ())?; + let section = Section::read_options(reader, endian, ())?; sections.push(section); } diff --git a/src/main.rs b/src/main.rs index 95aa29d9..7c934c09 100644 --- a/src/main.rs +++ b/src/main.rs @@ -6,7 +6,7 @@ // // SPDX-License-Identifier: MPL-2.0 -use binrw::{BinRead, ReadOptions}; +use binrw::BinRead; use clap::{Parser, Subcommand}; use rekordcrate::anlz::ANLZ; use rekordcrate::pdb::{Header, PageType, Row}; @@ -85,7 +85,7 @@ fn list_playlists(path: &PathBuf) -> rekordcrate::Result<()> { header .read_pages( &mut reader, - &ReadOptions::new(binrw::Endian::NATIVE), + binrw::Endian::NATIVE, (&table.first_page, &table.last_page), ) .unwrap() @@ -101,7 +101,6 @@ fn list_playlists(path: &PathBuf) -> rekordcrate::Result<()> { unreachable!("encountered non-playlist tree row in playlist table"); } }) - .cloned() .collect::>() .into_iter() }) @@ -132,7 +131,7 @@ fn dump_pdb(path: &PathBuf) -> rekordcrate::Result<()> { for page in header .read_pages( &mut reader, - &ReadOptions::new(binrw::Endian::NATIVE), + binrw::Endian::NATIVE, (&table.first_page, &table.last_page), ) .unwrap() diff --git a/src/pdb/mod.rs b/src/pdb/mod.rs index aa5f44c6..44a6b09f 100644 --- a/src/pdb/mod.rs +++ b/src/pdb/mod.rs @@ -24,12 +24,13 @@ use crate::pdb::string::DeviceSQLString; use crate::util::ColorIndex; use binrw::{ binread, binrw, + file_ptr::FilePtrArgs, io::{Read, Seek, SeekFrom, Write}, - BinRead, BinResult, BinWrite, Endian, FilePtr16, FilePtr8, ReadOptions, WriteOptions, + BinRead, BinResult, BinWrite, Endian, FilePtr16, FilePtr8, }; /// Do not read anything, but the return the current stream position of `reader`. -fn current_offset(reader: &mut R, _: &ReadOptions, _: ()) -> BinResult { +fn current_offset(reader: &mut R, _: Endian, _: ()) -> BinResult { reader.stream_position().map_err(binrw::Error::Io) } @@ -161,7 +162,7 @@ impl Header { pub fn read_pages( &self, reader: &mut R, - ro: &ReadOptions, + endian: Endian, args: (&PageIndex, &PageIndex), ) -> BinResult> { let (first_page, last_page) = args; @@ -171,7 +172,7 @@ impl Header { loop { let page_offset = SeekFrom::Start(page_index.offset(self.page_size)); reader.seek(page_offset).map_err(binrw::Error::Io)?; - let page = Page::read_options(reader, ro, (self.page_size,))?; + let page = Page::read_options(reader, endian, (self.page_size,))?; let is_last_page = &page.page_index == last_page; page_index = page.next_page.clone(); pages.push(page); @@ -312,7 +313,7 @@ impl Page { /// Parse the row groups at the end of the page. fn parse_row_groups( reader: &mut R, - ro: &ReadOptions, + endian: Endian, args: (PageType, u64, u16, u16, PageFlags), ) -> BinResult> { let (page_type, page_heap_offset, num_rows, num_row_groups, page_flags) = args; @@ -331,7 +332,7 @@ impl Page { // Read last row group let row_group = RowGroup::read_options( reader, - ro, + endian, (page_type, page_heap_offset, num_rows_in_last_row_group), )?; row_groups.push(row_group); @@ -340,7 +341,7 @@ impl Page { for _ in 1..num_row_groups { let row_group = RowGroup::read_options( reader, - ro, + endian, (page_type, page_heap_offset, RowGroup::MAX_ROW_COUNT), )?; row_groups.insert(0, row_group); @@ -395,7 +396,7 @@ pub struct RowGroup { /// An offset which points to a row in the table, whose actual presence is controlled by one of the /// bits in `row_present_flags`. This instance allows the row itself to be lazily loaded, unless it /// is not present, in which case there is no content to be loaded. - #[br(offset = page_heap_offset, args { count: num_rows.into(), inner: (page_type,) })] + #[br(args { count: num_rows.into(), inner: FilePtrArgs { inner: (page_type,), offset: page_heap_offset }})] rows: Vec>, row_presence_flags: u16, /// Unknown field, probably padding. @@ -408,14 +409,14 @@ impl RowGroup { const MAX_ROW_COUNT: u16 = 16; /// Return the ordered list of row offsets that are actually present. - pub fn present_rows(&self) -> impl Iterator { + pub fn present_rows(&self) -> impl Iterator + '_ { self.rows .iter() .rev() .enumerate() .filter_map(|(i, row_offset)| { if (self.row_presence_flags & (1 << i)) != 0 { - row_offset.value.as_ref() + Some(row_offset.value.clone()) } else { None } @@ -835,48 +836,48 @@ impl binrw::meta::WriteEndian for Track { } impl BinWrite for Track { - type Args = (); + type Args<'a> = (); fn write_options( &self, writer: &mut W, - options: &WriteOptions, - _args: Self::Args, + endian: Endian, + _args: Self::Args<'_>, ) -> BinResult<()> { - debug_assert!(options.endian() == Endian::Little); + debug_assert!(endian == Endian::Little); let base_position = writer.stream_position()?; - self.unknown1.write_options(writer, options, ())?; - self.index_shift.write_options(writer, options, ())?; - self.bitmask.write_options(writer, options, ())?; - self.sample_rate.write_options(writer, options, ())?; - self.composer_id.write_options(writer, options, ())?; - self.file_size.write_options(writer, options, ())?; - self.unknown2.write_options(writer, options, ())?; - self.unknown3.write_options(writer, options, ())?; - self.unknown4.write_options(writer, options, ())?; - self.artwork_id.write_options(writer, options, ())?; - self.key_id.write_options(writer, options, ())?; - self.orig_artist_id.write_options(writer, options, ())?; - self.label_id.write_options(writer, options, ())?; - self.remixer_id.write_options(writer, options, ())?; - self.bitrate.write_options(writer, options, ())?; - self.track_number.write_options(writer, options, ())?; - self.tempo.write_options(writer, options, ())?; - self.genre_id.write_options(writer, options, ())?; - self.album_id.write_options(writer, options, ())?; - self.artist_id.write_options(writer, options, ())?; - self.id.write_options(writer, options, ())?; - self.disc_number.write_options(writer, options, ())?; - self.play_count.write_options(writer, options, ())?; - self.year.write_options(writer, options, ())?; - self.sample_depth.write_options(writer, options, ())?; - self.duration.write_options(writer, options, ())?; - self.unknown5.write_options(writer, options, ())?; - self.color.write_options(writer, options, ())?; - self.rating.write_options(writer, options, ())?; - self.unknown6.write_options(writer, options, ())?; - self.unknown7.write_options(writer, options, ())?; + self.unknown1.write_options(writer, endian, ())?; + self.index_shift.write_options(writer, endian, ())?; + self.bitmask.write_options(writer, endian, ())?; + self.sample_rate.write_options(writer, endian, ())?; + self.composer_id.write_options(writer, endian, ())?; + self.file_size.write_options(writer, endian, ())?; + self.unknown2.write_options(writer, endian, ())?; + self.unknown3.write_options(writer, endian, ())?; + self.unknown4.write_options(writer, endian, ())?; + self.artwork_id.write_options(writer, endian, ())?; + self.key_id.write_options(writer, endian, ())?; + self.orig_artist_id.write_options(writer, endian, ())?; + self.label_id.write_options(writer, endian, ())?; + self.remixer_id.write_options(writer, endian, ())?; + self.bitrate.write_options(writer, endian, ())?; + self.track_number.write_options(writer, endian, ())?; + self.tempo.write_options(writer, endian, ())?; + self.genre_id.write_options(writer, endian, ())?; + self.album_id.write_options(writer, endian, ())?; + self.artist_id.write_options(writer, endian, ())?; + self.id.write_options(writer, endian, ())?; + self.disc_number.write_options(writer, endian, ())?; + self.play_count.write_options(writer, endian, ())?; + self.year.write_options(writer, endian, ())?; + self.sample_depth.write_options(writer, endian, ())?; + self.duration.write_options(writer, endian, ())?; + self.unknown5.write_options(writer, endian, ())?; + self.color.write_options(writer, endian, ())?; + self.rating.write_options(writer, endian, ())?; + self.unknown6.write_options(writer, endian, ())?; + self.unknown7.write_options(writer, endian, ())?; let start_of_string_section = writer.stream_position()?; debug_assert_eq!(start_of_string_section - base_position, 0x5e); @@ -919,12 +920,12 @@ impl BinWrite for Track { message: "Wraparound while calculating row offset".to_string(), })?; string_offsets[i] = offset; - string.write_options(writer, options, ())?; + string.write_options(writer, endian, ())?; } let end_of_row = writer.stream_position()?; writer.seek(SeekFrom::Start(start_of_string_section))?; - string_offsets.write_options(writer, options, ())?; + string_offsets.write_options(writer, endian, ())?; writer.seek(SeekFrom::Start(end_of_row))?; Ok(()) diff --git a/src/setting.rs b/src/setting.rs index 633df199..7f616e8d 100644 --- a/src/setting.rs +++ b/src/setting.rs @@ -22,7 +22,7 @@ //! The `SettingData` structs implement the `Default` trait and allows you to create objects that //! use the same default values as found in Rekordbox 6.6.1. -use binrw::{binrw, io::Cursor, BinWrite, Endian, NullString, WriteOptions}; +use binrw::{binrw, io::Cursor, BinWrite, Endian, NullString}; use parse_display::Display; #[binrw] @@ -68,7 +68,7 @@ pub struct Setting { /// details. #[br(temp)] #[bw(calc = no_checksum.then_some(0).unwrap_or_else(|| self.calculate_checksum()))] - checksum: u16, + _checksum: u16, /// Unknown field (apparently always `0000`). #[br(temp)] #[br(assert(unknown == 0))] @@ -138,7 +138,7 @@ where fn calculate_checksum(&self) -> u16 { let mut data = Vec::::with_capacity(156); let mut writer = Cursor::new(&mut data); - self.write_options(&mut writer, &WriteOptions::new(Endian::Little), (true,)) + self.write_options(&mut writer, Endian::Little, (true,)) .unwrap(); let start = match self.data { // In `DJMMYSETTING.DAT`, the checksum is calculated over all previous bytes, including diff --git a/src/util.rs b/src/util.rs index e07fc0de..4c0adc1c 100644 --- a/src/util.rs +++ b/src/util.rs @@ -70,7 +70,7 @@ pub(crate) mod testing { use binrw::{ meta::{ReadEndian, WriteEndian}, prelude::*, - Endian, ReadOptions, WriteOptions, + Endian, }; use pretty_assertions::assert_eq; use pretty_hex::pretty_hex; @@ -81,44 +81,45 @@ pub(crate) mod testing { }; } - pub fn test_roundtrip_with_args( + pub fn test_roundtrip_with_args<'a, T>( bin: &[u8], obj: T, - read_args: ::Args, - write_args: ::Args, + read_args: ::Args<'a>, + write_args: ::Args<'a>, ) where + ::Args<'a>: Clone, + ::Args<'a>: Clone, T: BinRead + BinWrite + PartialEq + core::fmt::Debug + ReadEndian + WriteEndian, { - let write_opts = WriteOptions::new(Endian::NATIVE); - let read_opts = ReadOptions::new(Endian::NATIVE); + let endian = Endian::NATIVE; // T->binary let mut writer = binrw::io::Cursor::new(Vec::with_capacity(bin.len())); - obj.write_options(&mut writer, &write_opts, write_args.clone()) + obj.write_options(&mut writer, endian, write_args.clone()) .unwrap(); assert_eq!(bin.len(), writer.get_ref().len()); assert_eq_hex!(&bin, &writer.get_ref()); // T->binary->T writer.set_position(0); - let parsed = T::read_options(&mut writer, &read_opts, read_args.clone()).unwrap(); + let parsed = T::read_options(&mut writer, endian, read_args.clone()).unwrap(); assert_eq!(obj, parsed); // binary->T let mut cursor = binrw::io::Cursor::new(bin); - let parsed = T::read_options(&mut cursor, &read_opts, read_args).unwrap(); + let parsed = T::read_options(&mut cursor, endian, read_args.clone()).unwrap(); assert_eq!(obj, parsed); // binary->T->binary writer.set_position(0); parsed - .write_options(&mut writer, &write_opts, write_args) + .write_options(&mut writer, endian, write_args.clone()) .unwrap(); assert_eq!(bin.len(), writer.get_ref().len()); assert_eq_hex!(&bin, &writer.get_ref()); } - pub fn test_roundtrip(bin: &[u8], obj: T) + pub fn test_roundtrip<'a, T>(bin: &[u8], obj: T) where - ::Args: Default, - ::Args: Default, - T: BinRead + BinWrite + PartialEq + core::fmt::Debug + ReadEndian + WriteEndian, + ::Args<'a>: Default + Clone, + ::Args<'a>: Default + Clone, + T: BinRead + BinWrite + PartialEq + core::fmt::Debug + ReadEndian + WriteEndian + Clone, { test_roundtrip_with_args( bin, diff --git a/tests/tests_pdb.rs.in b/tests/tests_pdb.rs.in index 9dadff02..75d265df 100644 --- a/tests/tests_pdb.rs.in +++ b/tests/tests_pdb.rs.in @@ -11,7 +11,7 @@ fn pdb_{name}() {{ println!("Table {{}}: {{:?}}", i, table.page_type); let pages = header.read_pages( &mut reader, - &ReadOptions::new(binrw::Endian::NATIVE), + binrw::Endian::NATIVE, (&table.first_page, &table.last_page), ) .expect("failed to read pages"); From b8a63087282c7d921655a24d98da9239cd5d6277 Mon Sep 17 00:00:00 2001 From: Kerstin Humm Date: Wed, 15 Nov 2023 13:53:27 +0100 Subject: [PATCH 2/4] (refactor): remove unnecessary Clone trait --- src/util.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util.rs b/src/util.rs index 4c0adc1c..e6546cd1 100644 --- a/src/util.rs +++ b/src/util.rs @@ -119,7 +119,7 @@ pub(crate) mod testing { where ::Args<'a>: Default + Clone, ::Args<'a>: Default + Clone, - T: BinRead + BinWrite + PartialEq + core::fmt::Debug + ReadEndian + WriteEndian + Clone, + T: BinRead + BinWrite + PartialEq + core::fmt::Debug + ReadEndian + WriteEndian, { test_roundtrip_with_args( bin, From 3f62afc81ae1a1554de5dc7eca2b1ee995d914e3 Mon Sep 17 00:00:00 2001 From: Kerstin Date: Thu, 23 Nov 2023 01:17:38 +0100 Subject: [PATCH 3/4] (refactor): Add comment about expensive clone Co-authored-by: Swiftb0y <12380386+Swiftb0y@users.noreply.github.com> --- src/pdb/mod.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/pdb/mod.rs b/src/pdb/mod.rs index 44a6b09f..503b5b4a 100644 --- a/src/pdb/mod.rs +++ b/src/pdb/mod.rs @@ -416,6 +416,10 @@ impl RowGroup { .enumerate() .filter_map(|(i, row_offset)| { if (self.row_presence_flags & (1 << i)) != 0 { + // TODO: the explicit clone is probably quite expensive + // but the simplest way to make the borrow checker happy for now. + // This is forced by the changes to FilePtr in binrw 0.12. + // We should investigate how to remove the clone in the future. Some(row_offset.value.clone()) } else { None From 78fc63119bb8323b064fdb310db001ed13f35c60 Mon Sep 17 00:00:00 2001 From: Jan Holthuis Date: Thu, 30 Nov 2023 19:59:06 +0100 Subject: [PATCH 4/4] chore(pdb): Remove trailing whitespace in comment --- src/pdb/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pdb/mod.rs b/src/pdb/mod.rs index 503b5b4a..8f4eeaa3 100644 --- a/src/pdb/mod.rs +++ b/src/pdb/mod.rs @@ -417,7 +417,7 @@ impl RowGroup { .filter_map(|(i, row_offset)| { if (self.row_presence_flags & (1 << i)) != 0 { // TODO: the explicit clone is probably quite expensive - // but the simplest way to make the borrow checker happy for now. + // but the simplest way to make the borrow checker happy for now. // This is forced by the changes to FilePtr in binrw 0.12. // We should investigate how to remove the clone in the future. Some(row_offset.value.clone())