[LLVMdev] subtle problem with inst_iterator

[Vladimir Prus]

Chris Lattner wrote:

> >   inline IIty operator*()  const { return BI; }
> >   inline IIty operator->() const { return operator*(); }
> >
> > So operator* works as if value_type is Instruction*, but operator-> works
> > as if value_type is Instruction. Hmm ;-)
>
> Yeah, fishy huh?  :)

Yea, a bit. I've decided that before changing that I'd better find other 
problems, if any, so I run inst_iterator via checks provided by 
Boost.Iterators.

First problem is that inst_iterator (and actually InstIterator class template) 
is not Assignable, because it has a reference data member, while standard 
requires all iterators to be assignable.

Second InstIterator is not DefaultConstructile, which is required from 
Forwarditerator.

Also, I get error because InstIterator::difference_type is not signed integer 
type (its defined as unsigned), but in this case the current standard does 
not say it's should be signed, though it looks reasonable and proposal for 
new version of standard require that.

> > > Probably the right thing to do would be to make the inst_iterator
> > > return a reference to the *instruction* itself, rather than a reference
> > > to the pointer.  This would make it work the same as standard ilist
> > > iterators.
> >
> > Yes, I though about this appoach as well but decided it can be too
> > disturbing for other code.
>
> I think it's the most correct and consistent want to do it.

Great.

> > > The reason why we have it return pointers right now is to populate
> > > worklists, which allows us to do:
> > >
> > > std::set<Instruction*> WorkList(inst_begin(F), inst_end(F));
> >
> > Maybe this can be rewritten like;
> >
> >     std::set<Instruction*> WorkList;
> >     std::transform(insn_begin(F), insn_end(F),
> > 	inserter(WorkList, WorkList.begin(), take_address);
> >
> > given proper definition of take_addres. Or maybe manual initialization
> > will okay.
> >
> > So, if you think making inst_iterator's operator* return reference to
> > instruction is better than making it return reference to pointer, I can
> > take a look at this.
>
> I do think it's the best way.  Once concern though, please change code
> like this:
>
> std::set<Instruction*> WorkList(inst_begin(F), inst_end(F));
>
> into code like:
>      std::set<Instruction*> WorkList;
>      for (inst_iterator I = inst_begin(F), E = inst_end(F); I != E; ++I)
>        WorkList.insert(&*I);
>
> ... instead of using transform.  It's the same # of lines of code and much
> easier to understand for people not familiar with higher-order C++.  It
> also avoid having to pull in a "take_address" functor.
>
> I don't think this really occurs often enough to be a serious problem
> anyway.  :)

I attach a new patch which fixes all of the problems. It's kinda large, must 
most changes are technical. Everything builds for me.

- Volodya
-------------- next part --------------
A non-text attachment was scrubbed...
Name: InstIterator.diff
Type: text/x-diff
Size: 16414 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20040427/b4a863b0/attachment.diff>