C ++ Распределение кучи и повторное использование памяти

Question

У меня есть небольшой фрагмент:

Action* newAction(Agent& a, ACTION_MODE& m)
{
  Action *new_action;
  do
  {
    new_action = new Action(a,m);
  }
  while (!state->addAction(new_action));
  return new_action;
}

state->addAction(Action *a); вернет true, если * a было добавлено, false, если * a не добавлено (проверяет, существует ли * a уже).

Теперь я знаю, что goto считаются злыми для многих людей, поэтому в приведенном выше фрагменте перераспределение new_action в каждом цикле без его удаления не так ли?Разве следующий фрагмент не будет более «разумным»?

retry :
    Action *new_action = new Action(a,m);
    if (state->addAction(new_action))
    {
      return new_action;
    }
    else
    {
      delete new_action;
      goto retry;
    }

Извините, если это кажется элементарным, но это то, о чем я уже давно задумался.Что правильно, удалить память и затем перераспределить, или я могу перераспределить мгновенно?

РЕДАКТИРОВАТЬ:

Это было бы лучше?

Action* newAction(Agent& a, ACTION_MODE& m)
{      
  // state will only allow an action to be added if it does not already exist
  Action *new_action = new Action(a,m);

  if (!new_action) return 0;

  while (!state->addAction(new_action))
  {
    delete new_action;
    new_action = new Action(a,m);
  }

  return new_action;
}

Вызывающий эту функцию ожидает новое действие, которое уже было добавлено в состояние, поэтому удаление должно происходить здесь.

Answer 1

В этом коде есть потенциальная утечка памяти, так как вы создаете новое действие каждый раз в цикле, но, если оно не добавляется, вы не освобождаете его. Лучше было бы переместить создание действия за пределы цикла с помощью чего-то вроде:

Action *newAction (Agent& a, ACTION_MODE& m) {
    Action *new_action = new Action(a,m);
    while (!state->addAction(new_action))
        ;
    return new_action;
}

Это более эффективно, чем непрерывное удаление и повторное создание действия, и его следует использовать в предпочтении.

Я также исправил тип возвращаемого значения, чтобы компилятор не жаловался, и вам, вероятно, следует ввести некую стратегию сбоя, если вы никогда не сможете добавить действие.

Что-то вроде:

Action *newAction (Agent& a, ACTION_MODE& m) {
    // Try to make action, return null if no go.

    Action *new_action = new Action(a,m);
    if (new_action == 0)
        return 0;

    // Try a limited number of times to add action to state.

    int limit = 10;
    while (!state->addAction(new_action)) {
        if (--limit == 0)
            break;
        // Optional sleep if desired.
    }

    // If that didn't work, delete action and return null.

    if (limit == 0) {
        delete new_action;
        return 0;
    }

    // Otherwise the action was added, return it.

    return new_action;
}

Answer 2

Разве следующий фрагмент не будет более "разумным"?

Нет.Это было бы гораздо разумнее:

do
{
  new_action = new Action(a,m);
}
while (!state->addActionOrDelete(new_action));

Где addActionOrDelete делает именно то, что говорит.Если вы собираетесь передать владение указателем на другую функцию, то эта другая функция должна взять на себя ответственность за владение этим указателем.Если он не может добавить его, то он должен удалить его.

Но, честно говоря, я не понимаю, почему вы все равно выделяете эти Action объекты в цикле.Изменится ли состояние state так, что добавление действия раньше не работало, но может работать позже?Если так, разве не имеет смысла иметь state->addAction, который будет блокировать до тех пор, пока действие не может быть добавлено?

И почему вообще выделение?Это похоже на код в стиле Java или C #.Просто сделайте это:

while(!state->addAction(Action(a,m))) ;

Это создаст временные Action объекты.Нет необходимости в выделении кучи.Или еще лучше:

Action theAction(a, m);
while(!state->addAction(theAction)) ;

Трудно ли скопировать эти объекты?

Answer 3

Если вы хотите сохранить текущий контекст кода, сделайте 2 небольших изменения:

  Action *new_action = 0;  //<-- (1) initialize to 0
  do
  {
    delete new_action;   // (2) delete the previous pointer
    new_action = new Action(a,m);
  }
  while (!state->addAction(new_action));

Редактировать : Из ответа @ paxdiablo я понял, что на самом деле вам не нужно постоянно выделять и освобождать память. Просто сделайте это, как он предложил:

Action *new_action = new Action(a,m);
while(!state->addAction(new_action));
return new_action;

[Примечание: выберите его ответ, если вы собираетесь принять мой.]

Answer 4

Использование shared_ptr или unique_ptr было бы более разумным.Вы не должны больше делать свое собственное распределение памяти в C ++.

В любом случае, я не думаю, что это хорошее место для использования goto.Вы реализуете цикл, и это именно то, для чего нужна конструкция цикла.

Вот как это сделать с unique_ptr:

unique_ptr<Action> new_action;
do {
  new_action.reset(new Action(a,m));
} while (!state->addAction(std::move(new_action)));  // accept by &&

Answer 5

Я бы написал так:

for ( ; ; ) {
    Action* new_action = new Action(a, m);
    if (state->addAction(new_action)) return new_action;
    delete new_action;
}

Если вам не нравится возвращаться из середины цикла, измените его на разрыв, но это означает, что вы также должны переместить объявление new_action вне цикла.

Я всегда стараюсь соединить new и delete. Наличие одного в этой функции и одного в вызываемой функции кажется мне плохой практикой.