<div dir="ltr">On Tue, May 14, 2013 at 3:27 PM, Edwin Vane <span dir="ltr"><<a href="mailto:edwin.vane@intel.com" target="_blank">edwin.vane@intel.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">

<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
  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.<br>


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

</blockquote><div><br></div><div>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?)</div>
<div><br></div><div style>Perhaps we can try to meet up in IRC so I can understand this better?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
  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.<br></blockquote><div><br></div>
<div style>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.</div>
<div style><br></div><div style>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.</div><div style><br></div><div style>
Cheers,</div><div style>/Manuel</div><div style><br></div><div style><br></div></div></div></div>