arg_iterator missing inc/dec operators

Reid,

After your recent redefinition of arg_iterator, it’s missing increment/decrement operators (which people typically expect to be defined for iterators). So some external code relying on this is broken. If it’s not intentional, would be nice to have it fixed.

Specific code that I is broken looks like this:

llvm::Function f;
foo(–f->arg_end()); // passing the last argument to the function.

Dmitry.

Reid,

After your recent redefinition of arg_iterator, it's missing
increment/decrement operators (which people typically expect to be
defined for iterators). So some external code relying on this is broken.
If it's not intentional, would be nice to have it fixed.

Specific code that I is broken looks like this:

llvm::Function f;
foo(--f->arg_end()); // passing the last argument to the function.

The predecrement / decrement operators *do* exist on this iterator: https://github.com/llvm-mirror/llvm/blob/master/include/llvm/ADT/ilist_iterator.h#L153

Maybe you meant:

   llvm::Function *f;
   foo(&*--f->arg_end());

?

This code doesn’t actually want to create and modify a temporary iterator. It’s better to rewrite this code as std::prev(f->arg_end()).

The predecrement / decrement operators *do* exist on this iterator:
https://github.com/llvm-mirror/llvm/blob/master/include/
llvm/ADT/ilist_iterator.h#L153

Maybe you meant:

  llvm::Function *f;
  foo(&*--f->arg_end());

?

Correct, I mean exactly this. Though &* is not important in this case.

This code doesn't work anymore, as arg_iterator is defined differently now:
https://github.com/llvm-mirror/llvm/blob/master/include/llvm/IR/Function.h#L57

I get an error when I'm using pre-decrement operator:
error: expression is not assignable
      --f->arg_end();

Dmitry.

Today's arg_iterator is now just an Argument*, which is the world's
simplest iterator. The '--f->arg_end()' snippet only compiled before this
change because we used to use fancy ilist iterators which had overloaded
decrement operators.

I don't think we should make a wrapper pointer iterator type just to make
old code like this compile.

Reid,

std::prev() works, though IMHO increment/decrement are more conventional way (I’m not saying better) to work with iterators.

I have no problem fixing code on my side, I’m just bringing your attention that code like this is broken. If your judgement is to keep it the way it is now, I’m ok with it.

Thanks.

Dmitry.

Reid,

std::prev() works, though IMHO increment/decrement are more conventional way (I’m not saying better) to work with iterators.

To be clear, increment/decrement do work with pointers, but the specific case here is that they don’t work with rvalue pointers. So this works:

iterator I = func();
–I;

but this does not:

–func()

^ is not guaranteed to work with all iterators (pointers being the obvious example - which would be true in a simple implementation of std::vector). It only happens to work with non-trivial iterators, where increment/decrement are implemented by operator overloads, rather than naturally by a pointer.

That’s why std::next/prev exist - to help with this case when writing portable/general code.

Actually, I suppose we could defensively disable this for non-trivial iterators by deleting rvalue overloads of the increment/decrement operators…