Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unintuitive AbstractTrees.children implementation #6

Open
pfitzseb opened this issue Jan 24, 2020 · 1 comment
Open

Unintuitive AbstractTrees.children implementation #6

pfitzseb opened this issue Jan 24, 2020 · 1 comment

Comments

@pfitzseb
Copy link

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.

@timholy
Copy link
Member

timholy commented Jan 24, 2020

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants