How could this code behave as I saw?

Question

I have a C++ application that had a one time assertion failure that I cannot reproduce. Here is the code that failed once:

unsigned int test(std::vector<CAction> actionQueue) {
  unsigned int theLastCount = actionQueue.size() - 1;

  std::vector<CAction>::const_reverse_iterator rItr = actionQueue.rbegin();
  std::vector<CAction>::const_reverse_iterator rEndItr = actionQueue.rend();

  for (; rItr != rEndItr; ++rItr, --theLastCount) {
    const CAction &fileAction = *rItr;

    if (fileAction.test()) {
      continue;
    }
    return theLastCount;
  }

  assert(theLastCount == 0); // How could this fail?

  return theLastCount;
}

Somehow, theLastCount was not zero after the loop completed.

From my reading of the logic, this should be impossible unless:

Some other thread side effected the actionQueue (which I don't think is possible).
Some transient memory corruption occurred.

Am I missing something stupid here, is there a bug in my code shown? Note that in the occurrence where I saw this, theLastCount should have been initialized to one as the vector had two elements.

But then the value is not tested. You can't fail a test if you don't take it. — Benjamin Lindley, Jan 15 '11 at 20:08

score 5 · Accepted Answer · answered Jan 15 '11 at 19:57

I believe that if test() passed for all fileActions, theLastCount would be -1. Consider:

theLastCount starts at actionQueue.size() -1. You decrement it once for each item in actionQueue- that is, it is now actionQueue.size() - 1 - actionQueue.size() = -1. Think about it. theLastCount keeps the index of the current iterator. But when the current iterator is rend, then that is one iterator before the beginning of the array- which is -1.

Edit: Oh, it's unsigned. But since you only test for equality to zero, then the integral overflow doesn't matter an awful lot here.

Question I need to figure out now, is why isn't the reaching end of loop reproducible, something else has gone wrong elsewhere just once and it uncovered this bug which was a fencepost not hit previously despite thousands of prior invocations. A case of the old one bug uncovering another bug lurking a long time in the code. — WilliamKF, Jan 15 '11 at 20:12

score 2 · Answer 2 · answered Jan 15 '11 at 19:57

2

If your actionQueue is empty, then

unsigned int theLastCount = actionQueue.size() - 1;

will set theLastCount to the maximum possible unsigned integer. The inner loop will never execute because the reverse iterators are equal to one another (rbegin() == rend() on empty containers), and so you'll hit the assertion with theLastCount equal to some staggeringly huge number.

answered Jan 15 '11 at 19:57

templatetypedef

362,284
104
897
1,065

Good point, true, however, in my case the function would have never been called in the first place if the vector were empty. – WilliamKF Jan 15 '11 at 19:59
1

More generally, though, it seems like if the loop runs to completion you'll always have the max unsigned value. You start the counter at `actionQueue.size() - 1` and then potentially decrement it `actionQueue.size()` times if you never hit the `return` statement in the loop. – templatetypedef Jan 15 '11 at 20:00

Nawaz · Answer 3 · 2011-01-15T20:13:33.943

1

void test(std::vector<CAction> actionQueue) 
{
  unsigned int theLastCount = actionQueue.size() - 1;
  /** Omitted code ***/
  {
    /** Omitted code ***/
    return theLastCount;
  }
  return theLastCount;
}

Forget about the error that you're unable to reproduce. But here is one serious problem. The return type is void, yet you return unsigned int!! How come?

I think, you need to write this:

assert(theLastCount == -1);//correct assert!

That is because if the test() pass for all elements, then theLastCount should become -1. Since there is no element left, and theLastCount always is valid element index if there is element. Else it should become -1.

NOTE : Change the type of theLastCount from unsigned int to int.

edited Jan 15 '11 at 20:13

answered Jan 15 '11 at 20:01

Nawaz

353,942
115
666
851

Mistake creating written version of code for this post, actual code returns unsigned int. – WilliamKF Jan 15 '11 at 20:02
@William: Okay. Then let me see what else is there! – Nawaz Jan 15 '11 at 20:03

score 1 · Answer 4 · answered Jan 15 '11 at 20:05

1

Please compile and run your code before you post it here! Firstly, this code doesn't compile (I'm not telling you why -- you can ask your compiler). Secondly, your assertion can never have succeeded, because theLastCount will always be (unsigned int)-1.

answered Jan 15 '11 at 20:05

TonyK

16,761
4
37
72

Code does compile, issue was the simplification of it for this posting. – WilliamKF Jan 15 '11 at 20:09

score 0 · Answer 5 · edited Aug 17 '12 at 13:05

0

What if the queue is empty?

theLastCount will be -1, and then... :-)

edited Aug 17 '12 at 13:05

Rostyslav Dzinko

39,424
5
49
62

answered Jan 15 '11 at 19:58

Reuben Scratton

38,595
9
77
86

How could this code behave as I saw?

5 Answers5