CODEWITHSUNDEEP
c++puzzlefencepost
WilliamKF
WilliamKF

Reputation: 43179

How could this code behave as I saw?

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:

  1. Some other thread side effected the actionQueue (which I don't think is possible).
  2. 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.

Upvotes: 3

Views: 179

Answers (5)

Reuben Scratton
Reuben Scratton

Reputation: 38717

What if the queue is empty?

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

Upvotes: 0

Sarfaraz Nawaz
Sarfaraz Nawaz

Reputation: 361612

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.

Upvotes: 1

TonyK
TonyK

Reputation: 17124

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.

Upvotes: 1

Puppy
Puppy

Reputation: 146968

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.

Upvotes: 5

templatetypedef
templatetypedef

Reputation: 372982

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.

Upvotes: 2

Related Questions