<div dir="ltr">First comments about 0001:<div><br></div><div>CopyFrom seems to assume the dest range is empty, or that there isn't any duplicates between the two (this and other), is that assumption safe on all accounts? If it is, shouldn't there be an assert? If not, shouldn't there be a collision detection?</div>
<div><br></div><div>The SingleLinkedListIterator<> seems a lot of boilerplate on the wrong place. Isn't there another container in LLVM that can do that? If not, shouldn't this code be in a more general header?</div>
<div><br></div><div>You have good comments, but they're not consistent. Some have \brief, others don't. Also, not all new functions have comments. While some functions are obvious, it serves the purpose of better Doxygen pages.</div>
<div><br></div><div><div>+void LiveInterval::RemoveEmptySubRanges() {</div><div>+  while (SubRanges != 0 && SubRanges->empty())</div><div>+    SubRanges = SubRanges->Next;</div><div><br></div></div><div>This looks like an over-optimization of special cases, that would probably be dealt with by the second loop, which is also over-complicated. If all you need is to jump the Next pointer when the SubRange is NULL, you can do that in a single loop, with less confusion, and less premature optimization.</div>
<div><br></div><div>For my education, SubRange::LaneMask is one bit per lane, right? So, 0x00000011 is the same as D0 or S0+S1, right?</div><div><br></div><div>The crucial function verifyLiveInterval() et al. seem ok, though I confess I haven't checked for all possible corner cases. I suspect you'll have tests for all of them.</div>
<div><br></div><div>cheers,</div><div>--renato</div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On 8 October 2013 15:00, Renato Golin <span dir="ltr"><<a href="mailto:renato.golin@linaro.org" target="_blank">renato.golin@linaro.org</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Hi Mathias,<div><br></div><div>I'm not sure how's the best way to review your patches. There are too many, and they're all related...</div>
<div><br></div><div>Would that be better on Phabricator? GitHub had a good way to review a series of patches on the same pull request, not sure Phabricator does the same thing.</div>
<div><br></div><div>I'm looking at them in order, but the emails will soon entangle when we start reviewing comments from different patches...</div><div><br></div><div>cheers,</div><div>--renato</div></div>
</blockquote></div><br></div>