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>