Ptr bookkeeping

[triptec]

I've added ptr bookkeeping to nodes, we should do some bookkeeping of namespaces as well but lets start here.

I suggest we first check if this fills the need of not leaking memory.

After that I suggest we first fix the api, I might have made some bad choices and there are questions, should add_* and import_node consume the node passed to them, should unlink() consume self etc.

When we feel the api makes sense lets document the methods etc.

I'm not suggesting we merge this anytime soon and I need comments on it to take this forward.

[dginev]

Excited to see this branch! I'll try to set aside some time and go through it carefully and leave comments/questions where they hit me.

[triptec]

I forgot that xmlFreeNode also frees the children of the node so we need to get all the children of a node that is unlinked and being dropped and remove them from the _Document.nodes

[triptec]

@dginev, any thoughts on this?

[dginev]

@triptec I was just looking yesterday again (after the huge period of silence), and I am feeling quite happy with the general direction. I branched into the local repo so that I can play around with it, wanted to more carefully examine the workflows that create a node and adopt it in a document, and of course to double-check if the memory deallocation is good. It also feels like a good place to clean up the "global deallocation" attempt that is sitting around as dead code.

Will drop some API comments on the change set as well.

[dginev]

I added the super easy bits to keep this PR 1) up-to-date with master, and 2) clean from warnings, clippy lints. You can see the single commit I've made so far at: https://github.com/KWARC/rust-libxml/commit/950c161714f5e2f6ee90402a1206ddd134f1e99b

It's on a re-forked branch from this PR, but in the main repository.

Interestingly, clippy had a friendly lint suggesting that certain methods do not consume node so they should take a (mutable) reference instead. I think there are some more methods to revise, but this is a start.

[dginev]

Had a long (too long really) session with valgrind tonight, and the base test suite is down to 0 bytes leaked. Commit: https://github.com/KWARC/rust-libxml/commit/99855f6a824104951a5c3a66f0e80faddb33bc79

This should give us at least some base sanity that Node operations are leak-free with the PR upgrades

[dginev]

Dropped entirely the global submodule, it was an early stabbed towards thread-safety, but it may be best to hold off on that until this refactor is complete - if at all. Commit: https://github.com/KWARC/rust-libxml/commit/e558e90def866155ed465a0fbaadb43494e04bb0

[dginev]

At this point I would ask @triptec if you would like to try rebasing your branch on master, and then merging my changes into it - or instead to open a new PR with the various small changes I added. Main benefit is that we can both commit to a branch in the same repository without much hassle.

[triptec]

@dginev, wouldn’t the simplest thing be if you open a pr of the branch you been working on, as you say we both would have access to that one?

[dginev]

@triptec a PR to the master of this repo, or a PR into your branch? The former I can do (essentially replace your PR with my branch) easily. But merging into your branch as a base "can not be done automatically" according to GitHub, unless you first rebase it on master. It has become tricky since master moved. I will open the easy PR, and we can take it from there.

[dginev]

I have opened #34 , let me know if this is what you had in mind.

Also, the time may be coming to split the tree pacakage into separate files - one per datastructure (Node, Document, Namespace, ...), as the current size is getting too large to manage - even the GitHub diff wouldn't load without an explicit request.

[dginev]

Ok, seems like we've come to agree to merge #34 asap, closing here to keep a single PR open.