Chris Lattner wrote:> On Fri, 23 Apr 2004, Vladimir Prus wrote: > > and since result of *it is considered to be rvalue it can't be accepted > > by this operator. The complete discussion is in > > > > http://std.dkuug.dk/jtc1/sc22/wg21/docs/papers/2002/n1385.htm > > > > I'd suggest to apply the following patch which makes operator* return > > reference to pointer. It makes my code compile and the rest of LLVM > > compiles too. Comments? > > Hrm, I'm not sure about this at all. In particular ilists have some funny > semantics that you should know about. In particular, the instruction list > is more like a std::list<Instruction> (but that allows polymorphism) than > a std::list<Instruction*>,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 ;-)> so it's not possible to just assign a new > instruction* through an iterator. In other words: *iit = new > Instruction(...) doesn't make any sense. > > 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.> 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. - Volodya
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/
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>