[llvm] r207743 - [LCG] Fix a bad bug in the new fancy iterator scheme I added to support

[Duncan P. N. Exon Smith]

On 2014-May-01, at 10:18, David Blaikie <dblaikie at gmail.com> wrote:

> On Thu, May 1, 2014 at 3:41 AM, Chandler Carruth <chandlerc at gmail.com> wrote:
>> Author: chandlerc
>> Date: Thu May  1 05:41:51 2014
>> New Revision: 207743
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=207743&view=rev
>> Log:
>> [LCG] Fix a bad bug in the new fancy iterator scheme I added to support
>> removal. We can't just blindly increment (or decrement) the adapted
>> iterator when the value is null because doing so can walk past the end
>> (or beginning) and keep inspecting the value. The fix I've implemented
>> is to restrict this further to a forward iterator and add an end
>> iterator to the members (replacing a member that had become dead when
>> I switched to the adaptor base!) and using that to stop the iteration.
>> 
>> I'm not entirely pleased with this solution. I feel like forward
>> iteration is too restrictive. I wasn't even happy about bidirectional
>> iteration. It also makes the iterator objects larger and the iteration
>> loops more complex. However, I also don't really like the other
>> alternative that seems obvious: a sentinel node. I'm still hoping to
>> come up with a more elegant solution here, but this at least fixes the
>> MSan and Valgrind errors on this code.
> 
> I hadn't noticed before, but it looks like you have a filtering
> iterator - again, would be nice to have that as a generic abstraction
> at some point. I know we have more than a few (specific_decl_iterator
> in Clang, to name one off the top of my head) cases of this.
> 
> (& FWIW, http://blogs.msdn.com/b/vcblog/archive/2010/08/09/the-filterator.aspx
> discusses how to write one and chooses the "store the current and end
> iterator in the filtering iterator" strategy - there's no other
> general solution, really)

IIRC, Boost's filtering iterator does the same.