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); 如果添加了 *a 将返回 true，如果未添加 *a 将返回 false（检查 *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;
}

如果您不喜欢从循环中间返回，请将其更改为break，但这意味着您还必须将new_action的声明移到循环之外。

我总是尝试将new和delete配对。在这个函数中拥有一个，在它调用的函数中拥有一个，对我来说似乎是一种不好的做法。