[PATCH] New and improved subexpression references implementation.

[Manuel Klimek]

On Tue, May 14, 2013 at 3:27 PM, Edwin Vane <edwin.vane at intel.com> wrote:

>
>   I think we must each have our own ideas of what this getNode<T> would
> do. I thought the idea was to search upward through the chain of parent
> builder links. For each builder, including the start one, we need to look
> through all the 'finished' matches stored in RecursiveBindings and
> Bindings. This implies some sort of traversal algorithm. This is what the
> iterator encapsulates. Putting this algorithm in getNode<T> won't make it
> any smaller.
>
>   The code could be reduced if all we cared about was the first node of
> matching Id and node type. In our discussions on 'combinatorial explosion',
> I thought we agreed this was preferable for usability. If so, that implies
> being able to get at matching nodes beyond the first and that's what the
> iterator is for.
>

I might be missing something :) In our discussion I actually thought that
we had ended up with only caring about one ID (at least for now). Can you
give a case where this is not enough (I skimmed the tests but didn't find
anything obvious - I assume the DISABLED part for the one test is
intentional?)

Perhaps we can try to meet up in IRC so I can understand this better?


>   In the first round of reviews there was also a comment about co-locating
> searching code so we could easily swap it out for something else in the
> future. The iterator also serves this purpose.
>

I thought the searching code would basically be a function on
BoundNodesTreeBuilder - as it seems to be tightly coupled to how that's
implemented anyway. I generally dislike iterators for anything that's not
given to a user - their implementation is complex, and corner cases are
hard to get right due to the state they imply, so I think we have to trade
off that maintenance cost against a significant benefit.

I'm still open to be sold on that benefit, but I unfortunately don't see it
yet... :( I'd rather start with the simplest possible solution.

Cheers,
/Manuel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130514/a0a1f0c2/attachment.html>