Segmentation fault in File I/O

I have written a code to read a file, store it in a structure and just display it. But somehow it is giving me a segmentation fault and I dont know why. Can someone please help me?

Output:

file: /home/neel/map2.txt
file opened
Start Intersection
a->road: 4

a->roadId[0]: 1
a->lane[0][0]: 2
a->lane[0][1]: 2

a->roadId[1]: 2
a->lane[1][0]: 2
a->lane[1][1]: 2

a->roadId[2]: 3
Segmentation fault

Code:

#include <iostream>
#include <fstream>
#include <stdio.h>
using namespace std;

struct Intersection
{
  unsigned short road;
  long long int *roadId;
  short *lane[2];
};

int main(int argc, char** argv)
{
  std::ifstream file;
  cout<<"file: "<<argv[1]<<endl;
  file.open(argv[1], std::ios::in);
  cout<<"file opened"<<endl;

  while (!file.eof())
  {
    cout<<"Start Intersection"<<endl;
    Intersection *a = new Intersection;
    file>>a->road;
    a->roadId = new long long int[a->road];
    a->lane[0] = new short[a->road];
    a->lane[1] = new short[a->road];
    cout<<"a->road: "<<a->road<<endl;
    for (int i=0; i<a->road; i++)
    {
      file>>a->roadId[i];

      cout<<endl<<"a->roadId["<<i<<"]: "<<a->roadId[i]<<endl;
      file>>a->lane[i][0];
      cout<<"a->lane["<<i<<"][0]: "<<a->lane[i][0]<<endl;
      file>>a->lane[i][1];
      cout<<"a->lane["<<i<<"][1]: "<<a->lane[i][1]<<endl;
  开发者_Python百科  }
    cout<<"Intersection inserted"<<endl;
    delete a;
  }
}

Text file:

4

1
2
2

2
2
2

3
2
2

4
2
2

---

Your lane is an array of 2 elements, however when i reaches 2 in your inner loop you are trying to print a->lane[2][0], which doesn't exist.

---

file>>a->lane[i][0];  //wrong
file>>a->lane[i][1];  //wrong

The indices should be reverse:

file>>(a->lane[0][i]); //correct
file>>(a->lane[1][i]); //correct

I added brackets just for clarity.

Besides, there is memory leak in your program. There should be as many delete as there are new statements, to ensure that there is no memory leak. So write these:

delete [] a->roadId;
delete [] a->lane[0];
delete [] a->lane[1];
delete a; //you've written only this!

Note delete a should be the last statement when deallocating the memory!

---

I don't mean to be nasty, but this code has enough problems it's almost difficult to decide which ones to start with.

using namespace std;

Here's the first red flag. About all I can say is that using namespace std; is a poor idea. With other namespaces it can be acceptable, but with std, it should always be avoided (IMO).

struct Intersection
{
  unsigned short road;
  long long int *roadId;
  short *lane[2];
};

This strikes me as a pretty poorly designed structure. std::vector is a good thing. Use it. Rather than just a structure of dumb data, you might want to consider defining operator>> for your structure type, so you can read one directly. Even if you don't do that, from the way you're using it, what you really want is something more like:

struct road { 
     long long Id;
     short lane[2];
};

struct Intersection {
    int road_count;
    road *roads;
};

Then, rather than a couple of parallel arrays all the same size that have to be walked in parallel, you get a a number of roads, each with its own data. std::vector is still better though.

int main(int argc, char** argv)
{
  std::ifstream file;
  cout<<"file: "<<argv[1]<<endl;
  file.open(argv[1], std::ios::in);

Rather than defining an ifstream object and then opening it separately, you should normally plan on passing the name to the ctor so it's defined and opened in a single operation, something like:

  std::ifstream file(argv[1]);

But, you also normally want to add a bit of error checking so you only attempt to use the command line argument as a file name if one has been passed, something like this:

  if (argc < 2) {
       std::cerr << "Usage: your_command <filename>\n";
       return EXIT_FAILURE;
  }

Then you'd have the code to define the ifstream.

  while (!file.eof())

Here's another major problem. A loop of this form is essentially always wrong (including this case, from the looks of things).

    cout<<"Start Intersection"<<endl;
    Intersection *a = new Intersection;

There seems to be no reason to allocate this dynamically. Are you, perhaps, a recovering (or perhaps not recovering) Java or C# programmer? Java requires that all objects of user defined classes be allocated dynamically, but C++ does not.

    file>>a->road;
    a->roadId = new long long int[a->road];
    a->lane[0] = new short[a->road];
    a->lane[1] = new short[a->road];
    cout<<"a->road: "<<a->road<<endl;
    for (int i=0; i<a->road; i++)
    {
      file>>a->roadId[i];

      cout<<endl<<"a->roadId["<<i<<"]: "<<a->roadId[i]<<endl;
      file>>a->lane[i][0];
      cout<<"a->lane["<<i<<"][0]: "<<a->lane[i][0]<<endl;
      file>>a->lane[i][1];
      cout<<"a->lane["<<i<<"][1]: "<<a->lane[i][1]<<endl;
    }

I'd prefer to separate the code for reading data from the code for displaying data. Except for things like homework (or debugging) you rarely want to display lots of raw data as you're reading it. In any case, the reading code should normally reside in the operator>> for that class, and the display code in operator<< for the class.

    cout<<"Intersection inserted"<<endl;

This seems to be an outright falsehood. You haven't actually inserted an Intersection into anything.

    delete a;

When you quit allocating the Intersection dynamically, you'll be able to eliminate this as well. If you insist on handling all the dynamic allocation by hand, you need to delete the components before this to avoid having memory leaks (another reason to prefer to std::vector).

I know that may sound pretty negative, and that leaves me a bit torn. On one hand, I'd really like to suggest better ways to do things. At the same time, this looks enough like homework that I'm extremely hesitant to just post better code either. I've tried to include a few hints about better ways, but realize they're probably not quite as specific as you'd like -- I apologize for that, but given that it's probably homework I don't think I can be a lot more specific.