Skip to content

Commit

Permalink
fix: use correct return value in bcf_get_format and bcf_get_info_valu…
Browse files Browse the repository at this point in the history 
…es (#398)

* fix: use correct return value in bcf_get_format and info

* test: check fs1 string vector length

* fix: sample count is 2 for test vcf file

* refactor: replace n with ret

* test: use shared buffer to test

* style: run cargo fmt

---------

Co-authored-by: Jialin Ma <jialin.ma@illumina.com>
  • Loading branch information
@Marlin-Na
Marlin-Na and Jialin Ma committed Jun 21, 2023
1 parent 261e3a3 commit f9a1981
Show file tree
Hide file tree
Showing 2 changed files with 52 additions and 41 deletions.
15 changes: 6 additions & 9 deletions src/bcf/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1022,15 +1022,12 @@ mod tests {
.expect("Missing tag")[0],
format!("string{}", i + 1).as_bytes()
);
println!(
"{}",
String::from_utf8_lossy(
record
.format(b"FS1")
.string()
.expect("Error reading string.")[0]
)
);
let fs1_str_vec = record
.format_shared_buffer(b"FS1", &mut buffer)
.string()
.expect("Error reading string.");
assert_eq!(fs1_str_vec.len(), 2);
println!("{}", String::from_utf8_lossy(fs1_str_vec[0]));
assert_eq!(
record
.format(b"FS1")
Expand Down
78 changes: 46 additions & 32 deletions src/bcf/record.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1251,7 +1251,7 @@ impl<'a, 'b, B: BorrowMut<Buffer> + Borrow<Buffer> + 'b> Info<'a, B> {
str::from_utf8(self.tag).unwrap().to_owned()
}

fn data(&mut self, data_type: u32) -> Result<Option<(usize, i32)>> {
fn data(&mut self, data_type: u32) -> Result<Option<i32>> {
let mut n: i32 = self.buffer.borrow().len;
let c_str = ffi::CString::new(self.tag).unwrap();
let ret = unsafe {
Expand All @@ -1270,7 +1270,7 @@ impl<'a, 'b, B: BorrowMut<Buffer> + Borrow<Buffer> + 'b> Info<'a, B> {
-1 => Err(Error::BcfUndefinedTag { tag: self.desc() }),
-2 => Err(Error::BcfUnexpectedType { tag: self.desc() }),
-3 => Ok(None),
ret => Ok(Some((n as usize, ret))),
ret => Ok(Some(ret)),
}
}

Expand All @@ -1284,9 +1284,10 @@ impl<'a, 'b, B: BorrowMut<Buffer> + Borrow<Buffer> + 'b> Info<'a, B> {
/// memory.
pub fn integer(mut self) -> Result<Option<BufferBacked<'b, &'b [i32], B>>> {
self.data(htslib::BCF_HT_INT).map(|data| {
data.map(|(n, ret)| {
let values =
unsafe { slice::from_raw_parts(self.buffer.borrow().inner as *const i32, n) };
data.map(|ret| {
let values = unsafe {
slice::from_raw_parts(self.buffer.borrow().inner as *const i32, ret as usize)
};
BufferBacked::new(&values[..ret as usize], self.buffer)
})
})
Expand All @@ -1302,9 +1303,10 @@ impl<'a, 'b, B: BorrowMut<Buffer> + Borrow<Buffer> + 'b> Info<'a, B> {
/// memory.
pub fn float(mut self) -> Result<Option<BufferBacked<'b, &'b [f32], B>>> {
self.data(htslib::BCF_HT_REAL).map(|data| {
data.map(|(n, ret)| {
let values =
unsafe { slice::from_raw_parts(self.buffer.borrow().inner as *const f32, n) };
data.map(|ret| {
let values = unsafe {
slice::from_raw_parts(self.buffer.borrow().inner as *const f32, ret as usize)
};
BufferBacked::new(&values[..ret as usize], self.buffer)
})
})
Expand All @@ -1313,7 +1315,7 @@ impl<'a, 'b, B: BorrowMut<Buffer> + Borrow<Buffer> + 'b> Info<'a, B> {
/// Get flags from tag. `false` if not set.
pub fn flag(&mut self) -> Result<bool> {
self.data(htslib::BCF_HT_FLAG).map(|data| match data {
Some((_, ret)) => ret == 1,
Some(ret) => ret == 1,
None => false,
})
}
Expand All @@ -1326,7 +1328,7 @@ impl<'a, 'b, B: BorrowMut<Buffer> + Borrow<Buffer> + 'b> Info<'a, B> {
/// memory.
pub fn string(mut self) -> Result<Option<BufferBacked<'b, Vec<&'b [u8]>, B>>> {

Check warning on line 1329 in src/bcf/record.rs

View workflow job for this annotation

GitHub Actions / clippy

very complex type used. Consider factoring parts into `type` definitions 

warning: very complex type used. Consider factoring parts into `type` definitions
    --> src/bcf/record.rs:1329:32
     |
1329 |     pub fn string(mut self) -> Result<Option<BufferBacked<'b, Vec<&'b [u8]>, B>>> {
     |                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     |
     = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#type_complexity
     = note: `#[warn(clippy::type_complexity)]` on by default
self.data(htslib::BCF_HT_STR).map(|data| {
data.map(|(_, ret)| {
data.map(|ret| {
BufferBacked::new(
unsafe {
slice::from_raw_parts(self.buffer.borrow().inner as *const u8, ret as usize)
Expand Down Expand Up @@ -1402,7 +1404,7 @@ impl<'a, 'b, B: BorrowMut<Buffer> + Borrow<Buffer> + 'b> Format<'a, B> {
}

/// Read and decode format data into a given type.
fn data(&mut self, data_type: u32) -> Result<(usize, i32)> {
fn data(&mut self, data_type: u32) -> Result<i32> {
let mut n: i32 = self.buffer.borrow().len;
let c_str = ffi::CString::new(self.tag).unwrap();
let ret = unsafe {
Expand All @@ -1423,7 +1425,7 @@ impl<'a, 'b, B: BorrowMut<Buffer> + Borrow<Buffer> + 'b> Format<'a, B> {
tag: self.desc(),
record: self.record.desc(),
}),
ret => Ok((n as usize, ret)),
ret => Ok(ret),
}
}

Expand All @@ -1434,12 +1436,17 @@ impl<'a, 'b, B: BorrowMut<Buffer> + Borrow<Buffer> + 'b> Format<'a, B> {
/// the BufferBacked object is already dropped, you will access unallocated
/// memory.
pub fn integer(mut self) -> Result<BufferBacked<'b, Vec<&'b [i32]>, B>> {
self.data(htslib::BCF_HT_INT).map(|(n, _)| {
self.data(htslib::BCF_HT_INT).map(|ret| {
BufferBacked::new(
unsafe { slice::from_raw_parts(self.buffer.borrow_mut().inner as *const i32, n) }
.chunks(self.values_per_sample())
.map(|s| trim_slice(s))
.collect(),
unsafe {
slice::from_raw_parts(
self.buffer.borrow_mut().inner as *const i32,
ret as usize,
)
}
.chunks(self.values_per_sample())
.map(|s| trim_slice(s))

Check warning on line 1448 in src/bcf/record.rs

View workflow job for this annotation

GitHub Actions / clippy

redundant closure 

warning: redundant closure
    --> src/bcf/record.rs:1448:22
     |
1448 |                 .map(|s| trim_slice(s))
     |                      ^^^^^^^^^^^^^^^^^ help: replace the closure with the function itself: `trim_slice`
     |
     = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#redundant_closure
     = note: `#[warn(clippy::redundant_closure)]` on by default
.collect(),
self.buffer,
)
})
Expand All @@ -1452,12 +1459,17 @@ impl<'a, 'b, B: BorrowMut<Buffer> + Borrow<Buffer> + 'b> Format<'a, B> {
/// the BufferBacked object is already dropped, you will access unallocated
/// memory.
pub fn float(mut self) -> Result<BufferBacked<'b, Vec<&'b [f32]>, B>> {
self.data(htslib::BCF_HT_REAL).map(|(n, _)| {
self.data(htslib::BCF_HT_REAL).map(|ret| {
BufferBacked::new(
unsafe { slice::from_raw_parts(self.buffer.borrow_mut().inner as *const f32, n) }
.chunks(self.values_per_sample())
.map(|s| trim_slice(s))
.collect(),
unsafe {
slice::from_raw_parts(
self.buffer.borrow_mut().inner as *const f32,
ret as usize,
)
}
.chunks(self.values_per_sample())
.map(|s| trim_slice(s))

Check warning on line 1471 in src/bcf/record.rs

View workflow job for this annotation

GitHub Actions / clippy

redundant closure 

warning: redundant closure
    --> src/bcf/record.rs:1471:22
     |
1471 |                 .map(|s| trim_slice(s))
     |                      ^^^^^^^^^^^^^^^^^ help: replace the closure with the function itself: `trim_slice`
     |
     = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#redundant_closure
.collect(),
self.buffer,
)
})
Expand All @@ -1470,17 +1482,19 @@ impl<'a, 'b, B: BorrowMut<Buffer> + Borrow<Buffer> + 'b> Format<'a, B> {
/// the BufferBacked object is already dropped, you will access unallocated
/// memory.
pub fn string(mut self) -> Result<BufferBacked<'b, Vec<&'b [u8]>, B>> {
self.data(htslib::BCF_HT_STR).map(|(n, _)| {
self.data(htslib::BCF_HT_STR).map(|ret| {
BufferBacked::new(
unsafe { slice::from_raw_parts(self.buffer.borrow_mut().inner as *const u8, n) }
.chunks(self.values_per_sample())
.map(|s| {
// stop at zero character
s.split(|c| *c == 0u8)
.next()
.expect("Bug: returned string should not be empty.")
})
.collect(),
unsafe {
slice::from_raw_parts(self.buffer.borrow_mut().inner as *const u8, ret as usize)
}
.chunks(self.values_per_sample())
.map(|s| {
// stop at zero character
s.split(|c| *c == 0u8)
.next()
.expect("Bug: returned string should not be empty.")
})
.collect(),
self.buffer,
)
})
Expand Down

0 comments on commit f9a1981

Please sign in to comment.