[PATCH] D29767: GraphTraits: Add range versions of graph traits functions (graph_nodes, graph_children, inverse_graph_nodes, inverse_graph_children).

[David Blaikie via llvm-commits]

On Thu, Feb 9, 2017 at 9:37 AM Daniel Berlin <dberlin at dberlin.org> wrote:

> On Thu, Feb 9, 2017 at 9:23 AM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> On Thu, Feb 9, 2017 at 8:14 AM Daniel Berlin via Phabricator <
> reviews at reviews.llvm.org> wrote:
>
> dberlin created this revision.
> Herald added a reviewer: tstellarAMD.
> Herald added subscribers: nhaehnle, arsenm.
>
> Convert all obvious node_begin/node_end and child_begin/child_end
> pairs to range based for.
>
> Sending for review in case someone has a good idea how to make
> graph_children able to be inferred. It looks like it would require
> changing GraphTraits to be two argument or something. I presume
> inference does not happen because it would have to check every
> GraphTraits in the world to see if the noderef types matched.
>
>
> Yep, that. One could use an extra trait for the node type to map it back
> to the graph trait to facilitate this - but that'd also mean each node type
> would have to be unique to the graph type (which is probably true - but one
> could imagine different graphs over the same nodes? Maybe... maybe not)
>
>
>
> We have different graphs over the same nodes already.
>
> The nodes of a graph of the function *are basicblock *
> the nodes of a graph of basicblock * are also basicblock *
>
> But they are not the same graph :)
>
> This only currently works through chicanery, IMHO. It would be nice to be
> able to define different graphs over the same types, but i'm not up for
> rewriting all of graph traits ATM.
>
>
>
>
> Did you consider changing the trait so they exposed range accessors
> directly, instead of adding utility functions?
>
>
> I can't figure out a way to do this.
>
> The base class traits have *nothing* in them.
>
> This was the best i could do without "rewrite graphtraits completely".
>
> "graph_children(graph)" is a bit of an awkward construct
>
>
> Note that if i put it in the traits, i believe this would get worse, not
> matter.
>

How so? Worse in terms of implementation complexity, or user complexity?

I imagine it would be worse for implementation complexity if it was added
to traits in a backwards compatible way (ie: where all exisitng traits
didn't have to change their API and would continue vending begins and ends,
rather than ranges). If all the traits had to change then I'm not sure of
the worsening that might cause.

(I suppose the inverse cases would remain non-members, creating an
undesirable asymmetry perhaps?)


>
>
> . Alternatively maybe drop the graph_ prefix - ADL/overload resolution/etc
> will make it correct anyway, so just "children(graph)", etc?
>
>
> I'm happy to drop the prefix.
>
>
>
> (re: your other comment, I'm not sure I quite followed:
>
> "Note that this use of const auto & is required to make clang build, as
> the iterator type it for CFG blocks ends up returning blocks directly
> otherwise here." - not sure I quite follow. Best guess/reading is that the
> CFG's iterators' value type is heavyweight, but most other GraphTraits
> types have lightweight nodes that are cheap to copy? (like pointers, etc)
>
>
> Sorta.
>
> First, graph traits doesn't work on anything but pointers.
>

Ah, I thought Tim (added Tim Shen to this thread) made this not be the case
anymore recently, I forget what the motivation was, and maybe it didn't end
up being needed/done.


>   Which is very very sad, but also outside the scope of this patch (and
> also means you can't, for example, have a graph of pairs or wahtever).
> Some day i will fix this.
>
> In any case, this particular issue is:
>
> the child iterators are expected to return noderefs, but it looks like the
> iterator for clang's CFG returns a value type.
>
> IE the iterators return, say, BasicBlock * (a pointer type).
> Clang's returns AdjacentCFGBlock  (a value type).
>

I'm confused then - sounds like a contradiction, that CFG does have a
NodeRef that's not a pointer.

*goes to look* Ah, I see, the NodeRef is a pointer type, but the child
dereference doesn't produce that, instead it produces something that's
convertible to a pointer. It's not much bigger - it's two pointers big
instead of one pointer.

And... I see what/why it's doing. Yeah, that. (ramble ramble) Yeah,
wouldn't be hard to add an iterator adapter in there to force the
conversion so its op*/value_type is /actually/ CFGBlock* instead of
something convertible to it.


>
> Thus, if i use "auto *" instead of "const auto &", it fails.
>

Right - from the comment I was reading it as "using const auto & instead of
auto" - wasn't thinking about "const auto *" as the alternative that was
being compared/contrasted.


> I believe clang is broken here, but honestly, it's hard to tell.
>

Fair.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170209/519ce96a/attachment.html>