inexplicable access violation

Although, I have been looking for hours in my program for memory management mistakes, without finding any, I keep getting the "access violation" error. According to the debugging, it seems to occur at an it++ line. Let me post you the whole code. If you gave me your lights, it would be highly appreciated.

Code:

---

//metro.h
#define totalWaggons 10
#define maxCapacity 40
using namespace std;
#include <list>

class train{
      public:
            train(int);
            ~train(void);
            void printStatistics(void);
            void operate();
            void income(int);
      private:
              int money;
              int stations;
};

class passenger{
      public:
            passenger(int, int);
            int have_ticket();
            int may_discount();
      private:
              int ticket;
              int discount;
};

class waggon{
      public:
            waggon();
            ~waggon();
            void embark(int);
            void disembark(int);
            void inStation();
            void betweenStations();
            void printStatistics(void);
      private:
              int offenders;
              int escapers;
              int passengers;
              list<class passenger *> passlist;
             
};

---

Code:

---

//metro.cpp
#include "metro.h"
#include <iostream>
#include <cstdlib>
#include <ctime>
train *T;

train::train(int s){
    cout << "A metro train with " << totalWaggons << " waggons, was created" << endl;
    stations = s;
    money=0;
    }
   
train::~train(){
    cout << "A metro train was destroyed" << endl;
    }

void train::operate(void){
    waggon* w;
    w = new waggon[totalWaggons];
    for(int i=0; i<stations; i++){
      for(int j=0; j<totalWaggons; j++){
        w[j].inStation();
        if(rand()%2)
          w[j].betweenStations();
        w[j].printStatistics();
      }
    printStatistics();
    }
    delete[] w;
} 

void train::income(int amount){
    money += amount;
}
   
waggon::waggon(void){
    cout << "A waggon with capacity for " << maxCapacity << " passengers, was created" << endl;
    passengers=offenders=escapers=0;
}
   
waggon::~waggon(void){
    cout << "A waggon was destroyed" << endl;
}

void waggon::printStatistics(void){
    cout << offenders << " passengers were found to offend" << endl;
    cout << escapers << " without ticket were not caught" << endl;
}

void train::printStatistics(void){
    cout << money << " euros were collected" << endl;
    }

void waggon::disembark(int n){
    list<passenger *>::iterator it;
    for(it=passlist.begin(); n>1; n--)
        it++;
    passlist.erase(it);
}

void waggon::embark(int n){
    list<passenger *>::iterator it;
    passenger *p;
    p = new passenger(rand()%2, rand()%2);
    if(!p->have_ticket())
          escapers++;
    else{
          if(p->may_discount()) T->income(1);
          else T->income(2);}
    for(it=passlist.begin(); n>1; n--)
        it++;
    passlist.insert(it, p);
}

void waggon::inStation(){
    int in, out, i, rnd;
    if(passengers){
        out = rand() % passengers + 1;
        for(i=0; i<out; i++){
          rnd = rand() % passengers +1;
          disembark(rnd);
          passengers--;
        }
    }
    in = rand() % (maxCapacity-passengers) + 1;
    if(passengers==0){
      embark(1);
      passengers++; in--;
    }
    for(i=0; i<in; i++){
        rnd = rand() % passengers +1;
        embark(rnd);
        passengers++;
    }
}

void waggon::betweenStations(){
    list< passenger *>::iterator it;
    it=passlist.begin();
    while(it!=passlist.end()){
        if((*it)->have_ticket() == 0){
          offenders++;
          escapers--;
          if((*it)->may_discount())
              T->income(30);
          else
              T->income(60);
          it = passlist.erase(it);
        }
        else
          it++;
    }
}

passenger::passenger(int tick, int disc){
  ticket = tick;
  disc = discount;
}

int passenger::have_ticket(){
    return ticket;
}

int passenger::may_discount(){
    return discount;
}
       
int main(int argc, char *argv[]){
    int stations;
    if(argc>1){
      srand(time(NULL));
      stations = atoi(argv[1]);
      if(stations>0){
          T = new train(stations);
          T->operate();
          }
    }
    else
    cout << "Please define number of stations" << endl;
    system("pause");
}

---

This doesn't look particularly safe:

Code:

---

    for(it=passlist.begin(); n>1; n--)
        it++;

---

Perhaps add an assert that n <= passlist.size() and then use std::advance instead of that loop.

I consider a function that treats a list as though it were an array (i.e. disembark) to be an abomination.

Code:

---

void waggon::embark(int n){
    list<passenger *>::iterator it;
    passenger *p;
    p = new passenger(rand()%2, rand()%2);
    if(!p->have_ticket())
          escapers++;
    else{
          if(p->may_discount()) T->income(1);
          else T->income(2);}
    for(it=passlist.begin(); n>1; n--)
        it++;
    passlist.insert(it, p);
}

---

Your test condition here does not do anything to prevent it from going beyond the end of the list. There is also nothing preventing you passing an invalid iterator to the insert function, you want to make sure it points to something valid before you try to insert into the list at that position.

Code:

---

//metro.h
#define totalWaggons 10
#define maxCapacity 40
using namespace std;
#include <list>

...

class waggon{
    ...
    list<class passenger *> passlist;
         
};

---

Don't put that using namespace statement in a header. If you need to do so, do it at the source level after all headers have been included. Explicitly qualify any objects that need it:

Code:

---

//metro.h
#define totalWaggons 10
#define maxCapacity 40
#include <list>

...

class waggon{
    ...
    std::list<class passenger *> passlist;
         
};

---

Your metro.cpp source file will need the using namespace std bit in that file.

Here are some suggestions, roughly in the order that I observed from your code:

• Avoid using #define to define constants. Use the const keyword; in this case they might be static const member variables.
• Do not place using directives in header files at file scope, or before a #include. That is, move using namespace std; to metro.cpp and fully qualify the names in metro.h instead (use std::list instead of list).
• Be const correct: if the member function does not change the observable state of the object, declare it const.
• Include the parameter names in the function prototype.
• passenger's have_ticket and may_discount member functions look like they should return bool rather than int.
• waggon probably should be named wagon
• The passlist member variable probably should be a std::list<passenger> instead. If you insist on keeping it as a std::list<passenger*>, and then also using dynamic memory allocation, then you should implement the copy constructor and copy assignment operator of the waggon (wagon) class, as well as properly implement the destructor. Failure to do so can result in the "access violation" error that you are facing now, should a waggon (wagon) object be copied.
• Avoid the use of global variables: the T global variable really should be a local variable in the global main function. It does not look like it needs to be a pointer, so don't make it a pointer.
• Use constructor initialiser lists, e.g.,
  
  Code:

  ---

  train::train(int s) : money(0), stations(s) {
    cout << "A metro train with " << totalWaggons << " waggons, was created" << endl;
}

  ---

• Indent your code a little better, making sure that the indent levels are consistent.
• Use container classes. You already used std::list, so why not use std::vector<waggon> instead of directly using dynamic memory allocation?
• Prefer pre-increment to post-increment, unless you are actually making use of the effects of post-increment.
• waggon::disembark appears to require random access. Perhaps a deque or vector will be more appropriate than a doubly linked list here.

Quote:

---

Originally Posted by iMalc

This doesn't look particularly safe:

Code:

---

    for(it=passlist.begin(); n>1; n--)
        it++;

---

Perhaps add an assert that n <= passlist.size() and then use std::advance instead of that loop.

I consider a function that treats a list as though it were an array (i.e. disembark) to be an abomination.

---

Since n ranges from 1 to number of passengers, therefore within the limits of the list, there is no need for such an assert. I used std::advance, but I still get access violation. :(

Quote:

---

Originally Posted by laserlight

[*]The passlist member variable probably should be a std::list<passenger> instead. If you insist on keeping it as a std::list<passenger*>, and then also using dynamic memory allocation, then you should implement the copy constructor and copy assignment operator of the waggon (wagon) class, as well as properly implement the destructor. Failure to do so can result in the "access violation" error that you are facing now, should a waggon (wagon) object be copied.

---

I am not sure I fully understand you. How shall I implement the copy constructor and assignment operator of the waggon class and how does this relate to the access violation errors? Could you show me by code?
Thanks for the rest of your comments. They will be taken into consideration.

Quote:

---

Originally Posted by vril

Since n ranges from 1 to number of passengers, therefore within the limits of the list, there is no need for such an assert.

---

You should assert anyway, just in case your assumption proves to be incorrect.

Quote:

---

Originally Posted by vril

How shall I implement the copy constructor and assignment operator of the waggon class and how does this relate to the access violation errors?

---

I would prefer just making the member variable a std::list<passenger>. The potential problem now is that if a waggon object is copied, its copy has a std::list of passenger pointers that point to the same passenger objects as the original waggon object. This might be fine, or it might not.

Quote:

---

Originally Posted by laserlight

I would prefer just making the member variable a std::list<passenger>. The potential problem now is that if a waggon object is copied, its copy has a std::list of passenger pointers that point to the same passenger objects as the original waggon object. This might be fine, or it might not.

---

I prefer to keep it as std::list<passenger *>. Is there any idea what causes the access violation and how it can be avoided?

Quote:

---

Originally Posted by vril

I prefer to keep it as std::list<passenger *>.

---

Why? I can tell you that doing it as per my preference is simply easier to be correct.

Quote:

---

Originally Posted by vril

Is there any idea what causes the access violation and how it can be avoided?

---

You have presumably updated your code based on the various suggestions that you have received. What is your current code? Furthermore, use a debugger, set breakpoints and step through your program. Where is the access violation triggered? This can help you to determine the start of the portion of your code which you can ignore when searching for this mistake (but it does not always tell you exactly where the problem itself lies: the problem could have surfaced due to something else).

Quote:

---

Originally Posted by vril

Since n ranges from 1 to number of passengers, therefore within the limits of the list, there is no need for such an assert.

---

If everyone thought like that then asserts would never get used!

n is a parameter to the function. It could get called from anywhere else later on and the value might be out of range. The assert acts as a precondition check. Not only that, but if you've ever worked in a team you'd realise that the assert has the effect of documenting the assumption your code is making so that others may see what you're assuming.
This is actually the perfect candidate for an assert.

Quote:

---

Originally Posted by iMalc

If everyone thought like that then asserts would never get used!

n is a parameter to the function. It could get called from anywhere else later on and the value might be out of range. The assert acts as a precondition check. Not only that, but if you've ever worked in a team you'd realise that the assert has the effect of documenting the assumption your code is making so that others may see what you're assuming.
This is actually the perfect candidate for an assert.

---

You were right. For some reason assertion fails...
edit: I found it! When a passenger witout ticket disembarked, I forgot to decrement total passengers. I fixed it and the program doesn't crash anymore. However, I am now facing another strange complication. The while-block inside betweenStations becomes an endless loop, as if the iterator never equals passlist.end.

I found it once again. It was because of advance(it, n), which result in deleting the end node of the list. I corrected it with advance(it, n-1).

Awesome, glad to hear it, and I'm sure it serves very well to demonstrate how asserts are wickedly great tool!

Use them whenever your program makes an assumption that if false, would result in your program misbehaving. You leave them in the final program by the way, they don't affect release code, only debug.