tags:

views:

169

answers:

7

I have an stl iterator resulting from a std::find() and wish to test whether it is the last element. One way to write this is as follows:

mine *match = someValue;
vector<mine *> Mine(someContent);
vector<mine *>::iterator itr = std::find(Mine.begin(), Mine.end(), match);

if (itr == --Mine.end()) {
  doSomething;
}

But it seems to me that decrementing the end() iterator is asking for trouble, such as if the vector has no elements, then it would be undefined. Even if I know it will never be empty, it still seems ugly. I'm thinking that maybe rbegin() is the way to go, but am not certain as to best way to compare the forward iterator with a reverse iterator.

+10  A: 

Do this:

// defined in boost/utility.hpp, by the way
template <typename Iter>
Iter next(Iter iter)
{
    return ++iter;
}

// first check we aren't going to kill ourselves
// then check if the iterator after itr is the end
if ((itr != Mine.end()) && (next(itr) == Mine.end()))
{
    // points at the last element
}

That is all. Never gives you undefined behavior, works on all iterators, good day.

Wrap it up for fun:

template <typename Iter, typename Cont>
bool is_last(Iter iter, const Cont& cont)
{
    return (iter != cont.end()) && (next(iter) == cont.end())
}

Giving:

if (is_last(itr, Mine))

If you're allergic to utility functions/nice looking code, do:

if ((itr != Mine.end()) && (itr + 1 == Mine.end()))

But you can't do it on non-random-access iterators. This one works with bidirectional iterators:

if ((itr != Mine.end()) && (itr == --Mine.end()))

And is safe since end() > itr by the first check.

GMan
Concerning reverse iterators: `Mine.rbegin().base() == Mine.end();`. Reverse iterators store an iterator to the next element from the one you get when it is dereferenced. You'd still need to increment it first.
UncleBens
As I mentioned in my comment to the OP, I think the whole idea of needing to know if `itr` points to the last element is a code smell. Something just isn't right with that. However, if you really need to do this, this is how I'd do it.
John Dibling
The first one is the closest to being correct, except that it unnecessarily dereferences only to reference again. Also, doesn't `back()` throw if the vector is empty?
Steven Sudit
GMan
UncleBens
`back` does `*--a.end()` (23.1.1, table 68) which is exactly what OP suggested plus an unsafe dereference.
Potatoswatter
@GMan: The problem is that, if `itr` is `Mine.end()`, you're not guaranteed that it's safe to dereference. For vectors, it happens to work, but for linked lists, maps and other types, it may or may not, depending on whether there's a sentinel node or null pointer. However, `itr` already casts to an element pointer, so just leave it be.
Steven Sudit
@Potatoswatter: You can decrement a forward_iterator?
Steven Sudit
@Steven: Again, that's why I give the disclaimer at the bottom.
GMan
@Steven: That's what the standard says. `back` is optional and it doesn't make sense in a forward sequence anyway.
Potatoswatter
+1 for the `next` solution
Default
btw, shouldn't you give a temporary value, as to not tamper with the original value? `T next(const }`
Default
-1 for still suggesting UB (`back` on an empty vector is illegal) and convolution (reverse iterators can't be justified).
Potatoswatter
@Michael: No need, as x is being passed by value, anyhow.
Steven Sudit
I'm moving the disclaimer to the top since people are apparently blind. No, I'll just give the best answer, forget options. @Michael: The copy is made in the parameter.
GMan
@Steven: ah. That's right :) I'm so used to sending by reference..
Default
Steven Sudit
@Potatoswatter: I'm not sure what you're talking about when you say reverse iterators cannot be justified. When you -1'd my answer basically said "there's no point in doing anything with a reverse iterator".
GMan
@GMan: The concept of a reverse iterator is simply extraneous to solving the problem. Also, using `std::advance` doesn't require Boost. The -1 is more about UB; now that you've added a check against it I'll undownvote…
Potatoswatter
@Potato: But the OP mentioned reverse iterators in his question, I was addressing that by saying "there's no point". `std::advance` is mutating, `next` is not and reads better.
GMan
Oops, thanks, I forgot that about `advance`. Not that it's worth discussing, but you did include `rbegin` in a completely unironic suggestion.
Potatoswatter
@GMan You can't do `itr + 1` if it's not random access can you?
Mark B
@Mark: Correct, it was sort of a trade-off thing.
GMan
+1  A: 

A better way would be to copy the iterator and then increment it. You can then test the incremented version against end(). If you're careful, you can use a post-increment to avoid the need to formally copy it.

  if (++vector<mine*>::iterator(itr) == Mine.end())

If itr could already be at the end:

  if (itr == Mine.end() || ++vector<mine*>::iterator(itr) == Mine.end())

Or, based on GMan's answer but a bit safer:

  if (Mine.Length() == 0 || itr == Mine.End() || &*itr == &Mine.back())

I just fixed the last one again, as I was wrong about &*.

Steven Sudit
How is that any safer?
GMan
@GMan: If the goal is to determine whether the iterator points to the last valid element, this code will work consistently. If there is any risk of it already pointing past that last valid element, you can change the predicate to first test whether `itr == Mine.end()`, with an OR.
Steven Sudit
@Steven: You mean "better" as "works with forward iterators" then?
GMan
@GMan: I mean better in that it works correctly for any container.
Steven Sudit
-1 for convolution, no additional safety, and not appearing to work. What's the point of comparing the original, non-incremented value to `end`?
Potatoswatter
@Potatoswatter: You're right: I just fixed it to pre-increment the copied iterator instead of post-incrementing. Thanks.
Steven Sudit
Rescinded, although I prefer my third option.
Potatoswatter
Your third option decrements end, so I can't favor it.
Steven Sudit
+1  A: 

This is essentially the same problem as deleting a node from a singly-linked list. You have to have two iterators, one that follows one node behind the other, so when the "forward" iterator gets to the node you want to delete (or whatever operation; in your case the desired node would be the end), the "following" iterator is pointing to the node before (in your case, this would be the final node).

rmeador
Yes, that's kind of what I was incoherently alluding to when I said that "If you're careful, you can use a post-increment to avoid the need to formally copy it." So long as you keep the previous value of the iterator, you can pre-increment the current value and have that available even as you operate on the previous value. It's a little bit tricky to set up correctly, though.
Steven Sudit
+1  A: 

Why do you need to do special behavior only if the item is the last one?

What about this. The plan is just to compare the address of the iterator's item with the address of the last item in the container, with a check to make sure the item is actually not already the end (making the back call safe):

if (itr != Mine.end() && &*itr == &Mine.back()) {
  doSomething;
}
Mark B
This is *almost* right, in that it'll fail if `Mine.length()` is 0.
Steven Sudit
@Steven How can `Mine.length()` be 0 if it found a valid iterator in the container that's not end? By definition that iterator points to an item and thus `length` can't be 0. Or do I miss something obvious?
Mark B
Well, the `find` method can return `Mine.end()` if nothing was found.
Steven Sudit
@Steven And the first part of my check where I test that find didn't return `end` catches that.
Mark B
@Mark: You're correct. If you edit your answer, I'll be able to upvote it.
Steven Sudit
+3  A: 

Yes, it's unsafe to decrement (or increment) end if the vector may be empty. It's even somewhat unsafe to do the same with a pointer, although you'll probably get away with it.

To be really safe, use subtraction and values known to be safe and valid:

if ( Mine.end() - itr == 1 )

For compatibility with all forward iterators (such as in slist, as opposed to random-access iterators of vector and deque), use

if ( std::distance( itr, Mine.end() ) == 1 )

or if you are concerned with performance but have bidirectional iterators (incl. any C++03 container)

if ( itr != Mine.end() && itr == -- Mine.end() )

or the truly anal case of only forward iterators and O(1) time,

if ( itr != Mine.end() && ++ container::iterator( itr ) == Mine.end() )

or if you are hellbent on cleverness to avoid naming the iterator class,

if ( itr != Mine.end() && ++ ( Mine.begin() = itr ) == Mine.end() )
Potatoswatter
This should work, and while the pointer arithmetic isn't all that clear, at least this avoids potentially trying to dereference a null pointer.
Steven Sudit
You changed your answer to make it worse: now it only works with vectors.
Steven Sudit
@Steven: It was wrong before. The question only involves vectors. The edit didn't change its compatibility anyway, as I only eliminated a (universal) operation.
Potatoswatter
I can't imagine that calculating `Mine.end() - itr` could be efficient on a linked list, even if it's legal.
Steven Sudit
@Steven: I didn't change that part. Anyway, I updated with compatibility options.
Potatoswatter
GMan
@Gman: yeah, thanks… I ended up with this oddity by trying to avoid naming the iterator class while working with standard tools… hmm
Potatoswatter
@Potato: That last one is pretty clever, nice.
GMan
+1  A: 

If you do:

if(itr != Mine.end() && itr == --Mine.end())

It should be fine. Because if itr is not at the end then there must be at least 1 element in the container and so end must yield a value result when decremented.

But if you still don't like that, there are lots of ways to do something equivalent, as all the other answers show.

Here's another alternative:

if(itr != Mine.end() && std::distance(Mine.begin(), itr) == Mine.size()-1)
TheUndeadFish
The second one's not bad.
Steven Sudit
In the second solution if the container is list-like, it'll have to walk the whole list to determine the distance.
Mark B
+1  A: 

Here's another potential solution:

template<class Iterator, class Container> bool is_last(Iterator it, const Container& cont)
{
    // REQUIREMENTS:
    // the iterator must be a valid iterator for `cont`
    if( it == cont.end() )
        return false;   // or throw if you prefer
    return (++it) == cont.end();
}
John Dibling
+1, didn't see your answer, sorry for stepping on your toes. We hive-minded the function signature, though, high-five.
GMan
I don't believe all control paths return a value... there should be `return false;` as the last line of the function, right?
rmeador
@John You need a tail return false in your template function.
WilliamKF
Thanks all for pointing out my error. Code fixed!
John Dibling
@GMan: We sure did. At least I can be a little more sure I got the signature right! :)
John Dibling
The final three lines could just be `return it == cont.end();`
Mark B
@Mark B: I wrote it more verbose to be a little clearer as to intent, but you're probably right -- that wasn't necessary. Changed code.
John Dibling