Я не могу понять, почему я получаю ошибку сегмента в моем коде - PullRequest
Новая
       6

Я не могу понять, почему я получаю ошибку сегмента в моем коде

0 голосов
/ 04 февраля 2020

Итак, у меня есть это 

void MovieGroup::add(Movie* m)
{
  cout << "Size = " << size << endl;
  if(size < MAX_COLLECTION){

    if(size == 0){
      collection[0] = m;
    }
    else{

      for(int i = 0; i<size; i++){

        if(collection[i]->getYear() >= m->getYear()){

          for(int j = size-1; j != i; j--){
            collection[j] = collection[j-1];
          }
          collection[i] = m;
          break;

        }

        else{
          collection[size] = m;
        }

      }
    }

  size++;

  }
}

Каждый раз, когда я пытаюсь запустить его, это вызывает ошибку сегментации. Цель этого - добавить объект mov ie в эту коллекцию, но отсортированный по году. Я действительно не уверен, где я незаконно обращаюсь к памяти. Любая помощь по этому вопросу будет принята с благодарностью.

Дополнительная информация: MAX_COLLECTION установлен на 64, я пытаюсь начать с задней части массива, чтобы сдвинуть каждый объект на 1

1 Ответ

1 голос
/ 04 февраля 2020

Линия collection[size] = m; находится не в том месте. Он должен быть вне l oop.

Кроме того, когда вы перемещаете элементы вокруг, вы теряете некоторые элементы, потому что вы не сдвигаете их все, поэтому вместо этого вы перезаписываете некоторые из них. .

Кроме того, вы должны использовать return вместо break после смещения элементов, в противном случае вам придется следить за тем, выполняет ли l oop сдвиг, чтобы код после l oop может действовать соответственно.

Попробуйте вместо этого: 

void MovieGroup::add(Movie* m)
{
  cout << "Size = " << size << endl;
  if (size < MAX_COLLECTION) {
    for (int i = 0; i < size; ++i) {
      if (collection[i]->getYear() >= m->getYear()) {
        for (int j = size-1; j >= i; --j) {
          collection[j+1] = collection[j];
        }
        collection[i] = m;
        ++size;
        return;
      }
    }
    collection[size] = m;
    ++size;
  }
}

С учетом сказанного, я бы настроил функцию еще на шаг, чтобы была только 1 точка вставки и приращения, а не 2 points: 

void MovieGroup::add(Movie* m)
{
  cout << "Size = " << size << endl;
  if (size < MAX_COLLECTION) {
    int index = size;
    for (int i = 0; i < size; ++i) {
      if (collection[i]->getYear() >= m->getYear()) {
        for (int j = size-1; j >= i; --j) {
          collection[j+1] = collection[j];
        }
        index = i;
        break;
      }
    }
    collection[index] = m;
    ++size;
  }
}

И затем я бы предложил использовать стандартные алгоритмы вместо ручных операций: 

#include <algorithm>
#include <iterator>

void MovieGroup::add(Movie* m)
{
  cout << "Size = " << size << endl;
  if (size < MAX_COLLECTION) {
    auto collection_end = std::next(collection, size);
    auto iter = std::find_if(collection, collection_end,
      [=](Movie *movie){ return movie->getYear() >= m->getYear(); }
    );
    if (iter != collection_end) {
      std::copy_backward(iter, collection_end, collection_end + 1);
    }
    *iter = m;
    ++size;
  }
}

И, наконец, вы можете рассмотреть вопрос об изменении collection на std::vector вместо массива фиксированной длины, тогда вы можете использовать std::vector::insert() и не беспокоиться о ручном смещении элементов или отслеживании size: 

#include <algorithm>

void MovieGroup::add(Movie* m)
{
  cout << "Size = " << size << endl;
  auto iter = std::find_if(collection.begin(), collection.end(),
    [=](Movie *movie){ return movie->getYear() >= m->getYear(); }
  );
  collection.insert(iter, m);
}
...