LeftChildRightSiblingTrees.jl icon indicating copy to clipboard operation
LeftChildRightSiblingTrees.jl copied to clipboard

Unintuitive AbstractTrees.children implementation

Open pfitzseb opened this issue 5 years ago • 1 comments

The current implementation simply returns the current Node, which of course works fine because it is iterable. It is also kind of confusing because children(node) === node looks strange. I briefly thought

children(n::Node) = (c for c in n)

would be nicer, but it turns out that doesn't work with AbstractTrees:

julia> AbstractTrees.print_tree(flamegraph())
FlameGraphs.NodeData(ip:0x0, 0x01, 1:299)
ERROR: MethodError: no method matching keys(::Base.Generator{Node{FlameGraphs.NodeData},var"#9#10"})
Closest candidates are:
  keys(::Core.SimpleVector) at essentials.jl:602
  keys(::Cmd) at process.jl:639
  keys(::Tuple) at tuple.jl:46
  ...
Stacktrace:
 [1] pairs(::Base.Generator{Node{FlameGraphs.NodeData},var"#9#10"}) at ./abstractdict.jl:132

An array comprehension is fine, of course, but also allocates an array.

So yeah, all in all I'm how actionable that complaint is, but the current behaviour was confusing for me at least.

pfitzseb avatar Jan 24 '20 11:01 pfitzseb

I can understand that. The "raw" for c in parent (i.e., not using AbstractTrees) hopefully seems more intuitive?

The AbstractTrees interface was obviously an add-on, I mostly wanted the printing.

timholy avatar Jan 24 '20 11:01 timholy