Segmentation fault (core dumped)

[bjustice54]

Okay well I'm not to advanced at C++, its only my 2 quarter in college taking Computer Science so please don't say anything that will go straight over my head haha. Well the code I have compiles just fine but when I got to run it I'm getting this seg fault error and I'm pretty sure it has something to do with the pointers in my code, if someone could just look at it and help me out id appropriate it. I have no idea how to use debuggers either and I dont really have the time learn it at the moment. Thanks
So what this code is trying to do is read a .dat file that has 3 columns the first two are numbers that represent cities and the third is the cost of a cable that is connected between them. It will read the file and then find the cheapest cost that has all cities connected some way or form.
My Code:

Code: Select all

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

class Cable{ public: int city1, city2, cost; bool solution; };

int costcompare(const void *x, const void *y) {
  if ((**(Cable **)x).cost < (**(Cable**)y).cost) return -1;
  if ((**(Cable **)x).cost > (**(Cable**)y).cost) return 1;
  return 0;
}

class Solver {
  Cable **cables;
  int ncables;
  int ncities;
  int *group;
  Cable **argv;
  int sum;
  int dummy;
  string buffer;
  fstream *fin;

public:
  //constructor
  Solver() {
    this->ncables = ncables;
    this->ncities = ncities;
    this->sum = sum;
    this->dummy = dummy;
    this->buffer = buffer;
    this->cables = cables;
    this->group = group;
    this->fin = fin;
    this->argv = argv;
  }

  //reads the file from main
  void readFile(char **argv) {
    fstream *fin = new fstream(argv[1], ios::in);
    if (fin->fail()) {
      cerr << "Cannot open file " << argv[1] << "\n";
      exit(0);
    }

    while (getline(*fin, buffer)) ncables++;
    fin->close();
    fin->clear();

    Cable *cables = new Cable[ncables];

    int sum = 0;
    fin->open(argv[1], ios::in);
    for (int i = 0; i < ncables; i++) {
      //cables[i] = new Cable();
      *fin >> cables[i].city1>> cables[i].city2 >> cables[i].cost;
      sum += cables[i].cost;
    }
    fin->close();
    fin->clear();
  }

  // Finds the number of cities
  void  findNcity () {
    int ncities = 0;
    for (int i=0; i < ncables; i++) {
      if (cables[i]->city1 > ncities) ncities = cables[i]->city1;
      if (cables[i]->city2 > ncities) ncities = cables[i]->city2;
      }
  }

  //gives the group their numbers
  void giveGroupnum() {
    int *group = new int[ncities + 1];
    for (int i=0; i <= ncities; i++) group[i] = i;
  }

  //the cycle
  bool isCycle(Cable *cable) {
    if (group[cable->city1] == group[cable->city2]) return false;
    int dummy = group[cable->city2];
    for (int i = 0; i <= ncities; i++)
      if (group[i] == dummy) group[i] = group[cable->city1];
    return true;
  }


  //Solves the problem
  void solve() {
    qsort(cables, ncables, sizeof(Cable*), costcompare);
    for (int i=0; i < ncables; i++) {
      if (isCycle(cables[i])) cables[i]->solution = true;
      else cables[i]->solution = false;
    }
    for (int i=0; i < ncables; i++){
      if (cables[i]->solution == true)
   cout << i << ": " << cables[i]->city1 << " "
        << cables[i]->city2 << " "
        << cables[i]->cost << "\n"; }
  }

};

int main(int argc, char **argv)
{
  if (argc != 2) {
    cout << "\nUsage: " << argv[0] << " \n";
    exit (0);
  }

  Solver mysolver;

  mysolver.readFile (argv);
  mysolver.findNcity ();
  mysolver.giveGroupnum();
  mysolver.solve();
  return 0;
}


[exomo]

Simple answer: Don't use pointers

Long answer: It is sometimes neccessary to use pointers, sometimes you can optimize a program for speed by using pointers, but I don't think speed is releavant in your case. Better avoid pointers wherever you can.

Code: Select all

int costcompare(const void *x, const void *y)
{
    if ((**(Cable **)x).cost < (**(Cable**)y).cost) return -1;
    if ((**(Cable **)x).cost > (**(Cable**)y).cost) return 1;
    return 0;
}

You don't use this function, but seriously, what are you trying to do here? Why void pointers that are casted to pointerpointers? You could use a function like this:

Code: Select all

int costcompare(const Cable &x, const Cable &y)
{
    if (x.cost < y.cost) return -1;
    if (x.cost > y.cost) return 1;
    return 0;
}

You could also implement compare operators for your Cable class, but that might be over your head.

Code: Select all

    Cable **cables; //Pointer to pointer is ugly. it seems to be some kind of array, so better use vector<Cable> for one dimensional or vector< vector <Cable> > for twodimensional array.
    int *group; // same here, better use vector<group>
    Cable **argv; // you don't ever use this, but it doesn't make much sense anyway
    fstream *fin; // you don't need a pointer for a filestream, you can just have an object. and you don't need it as class member, better use a local variable where you need it

Code: Select all

    Solver()
    {
        this->ncables = ncables;
        this->ncities = ncities;
        this->sum = sum;
        this->dummy = dummy;
        this->buffer = buffer;
        this->cables = cables;
        this->group = group;
        this->fin = fin;
        this->argv = argv;
    }

What is this supposed to do? You copy the (yet uninitialized) members over themselves. Better think of useful default values. Pointers should be set to NULL, but wait ... I told you not to use pointers.

Code: Select all

void readFile(char **argv)

Why do you pass argv? You only need the filename, so readFile(string filename) would be better.

Code: Select all

fstream *fin = new fstream(argv[1], ios::in);

I don't complain that you make a local variable fin here, but that makes it more obvious you don't need the class member fin. Again, don't make it a pointer and dynamically allocate. Just use

Code: Select all

ifstream fin(filename);

that should do it. If you new the object, you must rmember to delete it when you don't need it anymore, but you don't. Local variables are deleted automatically when they go out of scope. And if you don't use a pointer you can just use the . operator (instead of ->) and you don't need dereferencing.

Code: Select all

while (getline(*fin, buffer)) ncables++;
        fin->close();
        fin->clear();

        Cable *cables = new Cable[ncables];

        int sum = 0;
        fin->open(argv[1], ios::in);

Not very nice. It might work, but you don't have to read you file twice. If you use vectors as storage you can just push your new Objects in without knowing the size before. And why do you use a pointer to pointer if you only use it dereferenced. Read each line into a temporary Object and push_back it to your vector.

Now we get to the point why your program doesn't work. ncables is not properly initialized (to 0), it does not have a useful value. By the way, cables is a local variable, that means all your data you read in is lost.

I will stop here. There are more things, but I don't want to do all of your work. And it wouldn't be much good looking for mistakes in your code if you have to rewrite most of it anyways. If you use all of the above and don't use any pointers, you should be able to change the rest of your code in the same way. One last thing: You might want to use http://www.cplusplus.com/reference/algorithm/sort/ instead of qsort to sort your vector.