-
Notifications
You must be signed in to change notification settings - Fork 1
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
Added version comparison and tests #147
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I left some nits regarding some code duplications and little optimization.
2.1 Doesn't allow decompression of higher version files by lower version compressor (Should it?)
I think so. The frame format may be changed in the future, so parsing them with an outdated compressor version may lead to undefined behavior.
atsc/src/header.rs
Outdated
let current_version = env!("CARGO_PKG_VERSION").to_string(); | ||
trace!("Versions: c:{} h:{}", current_version, header.version); | ||
match compare(current_version.clone(), header.version.clone()) { | ||
Ok(Cmp::Lt) => panic!( | ||
"Can't decompress! File is version ({}) is higher than compressor version ({})!", | ||
header.version, current_version | ||
), | ||
Ok(Cmp::Eq | Cmp::Gt) => debug!("File version: {}", header.version), | ||
_ => panic!("Wrong version number!"), | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Both decode
and borrow_decode
contain the same code for verifying the version field. Making a func for this looks reasonable to me.
fn verify_header_versions(header: &CompressorHeader) {
let current_version = env!("CARGO_PKG_VERSION").to_string();
trace!("Versions: c:{} h:{}", current_version, header.version);
match compare(¤t_version, &header.version) {
Ok(Cmp::Lt) => panic!(
"Can't decompress! File is version ({}) is higher than compressor version ({})!",
header.version, current_version
),
Ok(Cmp::Eq | Cmp::Gt) => debug!("File version: {}", header.version),
_ => panic!("Wrong version number!"),
}
}
Co-authored-by: Bohdan Siryk <[email protected]>
Closes #146
This PR addresses issue #146 in the following way:
CompressorHeader
(Extracted fromCargo.toml
)2.1 Doesn't allow decompression of higher version files by lower version compressor (Should it?)