Returning a document silently produces corrupted documents.

[cyisfor]

Every node keeps a pointer to its document... a pointer. So that means documents cannot be copied. For instance:

unittest {
    Document foo() {
        return createDocument("<a>");
    }
    Document bar = foo();
    bar.root.appendChild(bar.createElement("fuuuuuuu"));
}

For whatever reason I can't fathom, this module does not allow removing a node from one document, when appending it, instead just erroring out. Removing that would be pretty harmless, since documents don't really do anything, but I dunno if there are any subtle gotchas there. But because the document must always be the same, this code snippet is an unmitigable error.

createDocument() creates and returns a Document, and in creating it, createDocument adds a root node, and a node for "a". Then foo() returns a Document, which silently copies the document that createDocument returned. bar is now "secretly blitted document 2" but the pointers of every single node in it haven't been rewritten. Thus bar.root.document_ is still document 1. bar.root.appendChild() therefore uses document 1, while bar.createElement() creates elements with document 2.

The end result is a document that has nodes from another document, and nothing can add elements to those nodes.

Possible solutions:

• upgrade clone() to be the "this(this)" copy constructor. That way instead of secretly throwing away the valid document and creating an invalid one, it will secretly descend through the entire node tree, secretly duplicating each one.
• Have createDocument returns a Document* since this module basically reimplements with structs, everything that classes already do.
• When appending a child, instead of flipping out if the documents are different, simply assign the child's document to the current one, and possibly detach() it from any old node tree it might be part of.

I like the third option, honestly. There's no reason for an unrecoverable error there. It's just move semantics by default. People who want true copies of nodes, can appendChild(node.clone()). It's very efficient to just move a node from one document to another this way, and there's no scaling problems like with stuff that has to recursively alter the node tree.

Only problem is that children of an appended child will still have the old document. But since that's not an error anymore, it doesn't really seem like a huge concern. And if it is a huge concern, and scaling is not an issue, then you can just recursively set the document_ of all the children. That would still be scads cheaper than cloning the document.

The second option is an okay one too. Classes are implemented in D that way, with everything being a pointer. Since this module already reinvents all that logic internally with pointers and alloc(), it could stop pretending to be a real struct on the surface and just return a Document*. I don't like it, but at least it would eliminate hard to find errors where a document is secretly, implicitly copied. You could set @disable this(this) if you returned a pointer, I think. But there might be other errors where the code requires a document be copied no matter what.

The first option is terrible. If there are move semantics by default, and you want to copy a node, it's just move(...,node.clone()). If there are copy semantics by default, then moving a node is entirely impossible, and everything can only be copied. That gets really annoying when you're trying to embed one document fragment in another document.

[cyisfor]

But wait, why would it be copied? D guarantees that if you return something that isn't already a variable, then it initializes the return address with it instead of copying. Yet that test keeps failing...

[marcioapm]

I think the only reason we need the document at all, is because of the allocator.
I have been wondering if it's worth the hassle, and I think we should change it so that it uses the GC by default instead, with the possibility of using a custom allocator, but in a way that won't require referencing the document on every node.

[cyisfor]

Honestly they've been working on allocators an awful lot in the core D code lately, and I would want to wait for that to pan out, and use that.

I gave a shot at switching to normal GC a couple times, and it's actually pretty tricky. The problem I had was with const vs non-const functions, and templates. Normally you can declare stuff inout to avoid duplicating const code all over the place, but you cannot instantiate templates with inout parameters, so ChildrenForward!(inout Node) wouldn't compile. Consts in D are weird, because you can't assign to them, even if you're assigning a const. So I ended up either saying "docstep1, docstep2, etc" or using the std.typecons.Rebindable template absolutely freaking everywhere. Or else using mutable pointers to consts instead of consts, which is how most languages do it transparently.

I dunno I'm kind of new at D, so I might have run into completely unrelated language gotchas while messing around with switching the DOM to use normal GC.