Add copy() to Document

[dginev]

Looks cool, but got me thinking on the naming conventions we have. Traditionally we have:

• libxml2 refers to this as copy
• the perl and ruby wrappers tend to refer to this as clone, and there is an interesting conversation to have about the ambiguity of clone() in a Rust context.
• Nokogiri also uses .dup as a synonym to clone

I have been thinking that it may be best (long-term) to stick to the Rust clone method for the wrapper's "deep copy" of objects, which would however require being very strict with borrowing of Node, Document etc. Right now doing a clone on a Node will clone the Rust data structure only, without cloning the underlying libxml2 C tree, and I think that may be confusing to new users of the wrapper.

So I'm quite happy to see this method fleshed out in the PR, but would probably prefer seeing it as an implementation of Rust's Clone trait: https://doc.rust-lang.org/std/clone/trait.Clone.html

I think that can be done safely for Document already, but would need some breaking changes to also be done for Node. And I have some dependent projects which would need to be updated. That said I think it is worth it for the wrapper to feel more Rust-like.

[triptec]

Something like that?

[dginev]

Indeed. Wondering if the copy function should be part of the public interface, but that is a minor point now that clone is available.

Btw I have the subtle premonition your clone_form method may be leaking memory, since I don't think the original document gets reliably deallocated when you overwrite the document pointer.

[dginev]

I think good to merge if clone_from only succeeds on target Document objects that have a NULL self.doc_ptr, and just fails with a panic! on any others.

It's a rare method to use in our setup in the first place I think. May be the simplest way to avoid the memory leak for now,

My point being that in:

self.doc_ptr = doc_ptr;

the self.doc_ptr is never explicitly deallocated and can leak in certain uses.

[triptec]

how to explicitly deallocate self.doc_ptr?

[dginev]

I think I would do exactly what you did for the source object - but with the reverse condition - as in:

if !self.doc_ptr.is_null() {
  panic!("Can only invoke clone_from on a Document struct with no pointer assigned.")
}

avoids the issue entirely

[triptec]

but wouldn't we want to raise if the libxml2 function returns a null pointer?

[dginev]

Yes, that too. I'm only suggesting adding one more guard to what your PR has, no need to remove anything

[dginev]

Thanks! Sorry for the back-and-forth, but I think we're getting somewhere. Can evolve it further going forward