search

KWARC / rust-libxml

Rust wrapper for libxml2
https://crates.io/crates/libxml
MIT License
76 stars 38 forks source link

Doc out of scope #10

Closed grray closed 8 years ago

grray commented 8 years ago

First commit has test, which demonstrates segmentation fault, second commit fixes error and makes that test impossible to compile, so test is removed in second commit. This change also makes #8 impossible to compile, so that was the real cause of problem there.

grray commented 8 years ago

Maybe even xpath::Context should own Document?

dginev commented 8 years ago

Thanks! I am merging for now, just so that we have the latest fix in master.

Owning the entire document is a tricky choice, I would intuitively consider it wrong, as you may want to use many contexts on the same document (e.g. in subsequent different xpath calls), so consuming the document may be an issue. On the other hand since libxml is doing the actual memory management, our Rust clones of Document are cheap... But that makes things additionally confusing.

If the lifetime-based API to Context ends up too confusing, we can switch to ownership of Document with some extra documentation. I would need to do some more exhaustive tests before we figure that out, and possibly ship a minor release to cargo afterwards.