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

[Daniel Berlin via llvm-commits]

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.


> . 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.  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).

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

I believe clang is broken here, but honestly, it's hard to tell.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170209/bcc67388/attachment.html>