On Sunday, 24 May 2026 at 21:50:28 UTC, mitgedanken wrote:
Wrong link 8)
Anyway, check out the GitHub link.
There are many issues with the code here are the most serious
ones:
## 1. Critical: Dangling Pointers Everywhere — `Node*` Points to
Stack Locals, Not GC Objects
This is the **fundamental design flaw** in the entire module. In
D, `class` is a reference type.
`Node n = new Node(...)` puts `n` (a reference) on the stack and
the object on the GC heap.
`&n` gives you a `Node*` — a pointer to the **stack-local
reference**, not to the GC object.
```d
Tree tree = new Tree();
TreePtr treeRef = &tree; // pointer to stack-local `tree`
Node op = new Node(treeRef, Symbol("+"), 0);
Node one = new Node(treeRef, Symbol("1"), 0, nullableNode(&op));
// ^^
// address of stack-local `op`
```
When `op` goes out of scope, `one._root._node` is a **dangling
pointer**. The unittests happen
to work because everything is in the same scope, but as library
code this is
**undefined behavior**.
**The fix:** `Node` is already a reference type. `Node`
references are already nullable
(`Node n = null`). The entire `NullableNode` struct is
unnecessary — you could just use
`Node` directly:
```d
// Instead of NullableNode wrapping Node*:
Node _root; // already nullable, already a reference to the GC
object
```
---
## 2. Critical: `opBinary("~")` with `NullableNode` Returns a
Dangling Pointer
```d
auto opBinary(string op : "~")(NullableNode rhs) {
Node node = new Node(
this._tree,
this._symbol ~ rhs.symbol.str,
this._precedence,
this._root,
this._left,
this._right
);
return NullableNode(&node); // ← node is LOCAL, &node
dangles after return
}
```
`node` is a local variable. `&node` points to the stack frame.
After the function returns,
the `NullableNode` holds a dangling pointer — immediate
use-after-free.
---
## 3. Critical: `NullableNode.toHash` Hashes the Stack Address,
Not Object Identity
```d
size_t toHash() const pure nothrow @safe {
return cast(size_t)cast(void*)this._node;
}
```
This hashes the address of the *reference variable on the stack*,
not the identity of the
GC heap object. Two `NullableNode`s referencing the same `Node`
object through different
local variables will have different hashes:
```d
Node n1 = new Node(...);
Node n2 = n1; // same object
auto a = nullableNode(&n1);
auto b = nullableNode(&n2);
assert(a == b); // FALSE — different stack addresses!
```
`opEquals` also compares with `is` (pointer equality), so this
same bug means two
references to the same object through different variables are
considered **not equal**.
---
## 4. Serious: `opEquals` / `toHash` Contract Violation on `Node`
```d
override bool opEquals(Object other) const {
return (n.repr() == this.repr()); // repr() includes
children, parent
}
override size_t toHash() const nothrow @safe {
// only hashes symbol + precedence
}
```
If you mutate a `Node`'s children after inserting it into an
associative array,
`toHash` stays the same but `opEquals` changes, violating the AA
invariant. The hash
and equality must agree on what constitutes identity.
---
## 5. Serious: `_setKind` Classifies a Parentless Single-Child
Node as `Branch`
```d
if (!this.hasParent && this.hasLeft && this.hasRight)
this._kind = Kind.Root; // requires BOTH children
else if (this.hasLeft || this.hasRight)
this._kind = Kind.Branch; // any children + parent →
branch
```
A node with no parent and only one child gets classified as
`Branch`, not `Root`.
This is probably wrong — a node at the top of a tree with a
single child should be
a `Root`.
---
## 6. Serious: `Tree.repr()` Only Follows One Path
```d
while (curr !is null && !curr.isLeaf) {
if (curr.hasLeft())
curr = curr.left.node; // always prefers left
else if (curr.hasRight())
curr = curr.right.node;
}
```
This traverses only the leftmost path, ignoring entire right
subtrees. A proper tree
representation needs to visit all nodes (pre-order, in-order,
post-order, or level-order).
---
## 7. Serious: `TreeCrawler.opApply` Only Visits One Level
```d
int opApply(scope int delegate(ref NodePtr) dg) {
if (auto node = this._state._current) {
if (node.hasLeft) { /* visit left child */ }
if (node.hasRight) { /* visit right child */ }
}
return 0;
}
```
This only visits the immediate children of the current node — it
doesn't recursively
traverse the tree. As a "crawler" it's misleading. It also
mutates `_state._current`
during iteration, making the crawler stateful in a confusing way.
