Hello, I think there's a rather subtle problem with the inst_iterator. It
declares its iterator category as std::bidirectional_iterator_tag but C++
standard requirements for forward iterator (which are included in
requirements for bidirection iterator), say that the type of expression
*r;
should be T&, where 'r' is the iterator and T is its value type. The
inst_iterator, though, returns pointer to Instruction, not a reference to a
pointer to instruction.
The above might sound like I've gone crazy reading C++ standard, but my real
reason is that with the current code the following:
for_each(inst_begin(f), inst_end(f), boost::bind(
apply_phi_semantics, result, _1, instruction2vertex));
does not compile, where 'apply_phi_semantics' is my function and
boost::bind
is a tool for binding function arguments described in
http://www.boost.org/libs/bind/bind.html
The reason it does not compile is somewhat contrived. for_each passes the
result of *it to functional object returned by boost::bind. The operator() of
that object has the following signature:
template<class Arg1>
.... operator()(Arg& a1)
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?
- Volodya
-------------- next part --------------
A non-text attachment was scrubbed...
Name: iterator.diff
Type: text/x-diff
Size: 1245 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20040423/a6c17e60/attachment.diff>
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*>, 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. 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)); Which is nice. What do you think? -Chris -- http://llvm.cs.uiuc.edu/ http://www.nondot.org/~sabre/Projects/
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