[LLVMdev] subtle problem with inst_iterator

[Chris Lattner]

On Fri, 23 Apr 2004, Vladimir Prus wrote:
> Yea, I've noticed that. However, it looks like inst_iterator is iterator over
> pointers. Oh, wait a minite, that's the current code:
>
>   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?  :)

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

> > 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.  :)

Thanks,

-Chris

-- 
http://llvm.cs.uiuc.edu/
http://www.nondot.org/~sabre/Projects/