search

rust-bio / rust-htslib

This library provides HTSlib bindings and a high level Rust API for reading and writing BAM files.
MIT License
302 stars 77 forks source link

bcf::Record::info should not take `&mut self` #78

Open holtgrewe opened 6 years ago

holtgrewe commented 6 years ago

Rather, there should be a mut and non-mut Info type similar to how there is iter() and iter_mut().

@johanneskoester what do you think?

johanneskoester commented 6 years ago

Yes, in particular since we discovered that htslib is copying the info information out of the record object, right? So the whole mutable Info functionality is obsolete? Or am I missing something?

felix-clark commented 4 years ago

By my reading of the docs, the Info struct does not expose any mutable references and its interface is functionally read-only. The INFO field of the BCF data is modified instead through the clear_info_*() and push_info_*() functions of Record. Unless there is some other issue in the implementation it looks like Info's public functions, along with its private data() function, can be changed to take immutable references to self.

The difficulty appears to be in Info::data()'s call to htslib::bcf_get_info_values(), an auto-generated function which takes mutable pointers. The underlying HTSlib C function does not appear to be intended to edit the info data, so maybe it should instead take pointers to const. Perhaps this can be worked around with interior mutability, but such an update would likely be non-trivial and introduce concerns of strict safety guarantees.

felix-clark commented 2 years ago

This appears to have been dealt with since version 0.35. Just wanted to point out that this issue can probably be closed.