On Tue, May 14, 2013 at 3:27 PM, Edwin Vane <[email protected]> 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
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
