C++ 成员函数中的“if (!this)”有多糟糕？

Question

如果我在应用程序中遇到 if (!this) return; 的旧代码，这是多么严重的风险？它是一个危险的定时炸弹，需要立即在整个应用程序范围内进行搜索和销毁工作，还是更像是一种可以安静地留在原地的代码气味？

当然，我不打算编写代码来做到这一点。相反，我最近在我们的许多应用程序使用的旧核心库中发现了一些东西。

想象一个CLookupThingy 类有一个非虚拟 CThingy *CLookupThingy::Lookup( name ) 成员函数。显然，在那个牛仔时代，其中一位程序员遇到了许多从函数传递 NULL CLookupThingy *s 的崩溃，他没有修复数百个调用站点，而是悄悄地修复了 Lookup()：

CThingy *CLookupThingy::Lookup( name ) 
{
   if (!this)
   {
      return NULL;
   }
   // else do the lookup code...
}

// now the above can be used like
CLookupThingy *GetLookup() 
{
  if (notReady()) return NULL;
  // else etc...
}

CThingy *pFoo = GetLookup()->Lookup( "foo" ); // will set pFoo to NULL without crashing

本周早些时候我发现了这颗宝石，但现在我对是否应该修复它感到矛盾。这是我们所有应用程序使用的核心库。其中一些应用程序已经交付给数百万客户，而且似乎运行良好；该代码没有崩溃或其他错误。删除查找函数中的if !this 将意味着修复数千个可能传递NULL 的呼叫站点；不可避免地会遗漏一些，引入新的错误，这些错误将在接下来的年开发中随机出现。

因此，除非绝对必要，否则我倾向于不理会它。

鉴于它在技术上是未定义的行为，if (!this) 在实践中有多危险？是否值得花费人工数周的时间来修复，还是可以指望 MSVC 和 GCC 安全返回？

我们的应用程序在 MSVC 和 GCC 上编译，并在 Windows、Ubuntu 和 MacOS 上运行。对其他平台的可移植性是无关紧要的。保证有问题的函数永远不会是虚拟的。

编辑：我正在寻找的客观答案类似于

• “当前版本的 MSVC 和 GCC 使用 ABI，其中非虚拟成员实际上是带有隐式 'this' 参数的静态变量；因此即使 'this' 为 NULL，它们也会安全地分支到函数中”或
• “即将发布的 GCC 版本将更改 ABI，因此即使是非虚拟函数也需要从类指针加载分支目标”或
• “当前的 GCC 4.5 有一个不一致的 ABI，有时它将非虚拟成员编译为带有隐式参数的直接分支，有时编译为类偏移函数指针。”

前者意味着代码很臭但不太可能破解；第二个是编译器升级后要测试的东西；后者需要立即采取行动，即使代价高昂。

显然，这是一个等待发生的潜在错误，但现在我只关心减轻我们特定编译器的风险。

Answer 1

我会不理它的。作为SafeNavigationOperator 的老式版本，这可能是经过深思熟虑的选择。正如您所说，在新代码中，我不会推荐它，但对于现有代码，我会不理会它。如果你最终修改它，我会确保所有对它的调用都被测试很好地覆盖。

编辑添加：您可以选择仅在代码的调试版本中通过以下方式将其删除：

CThingy *CLookupThingy::Lookup( name ) 
{
#if !defined(DEBUG)
   if (!this)
   {
      return NULL;
   }
#endif
   // else do the lookup code...
}

因此，它不会破坏生产代码的任何内容，同时让您有机会在调试模式下对其进行测试。

Answer 2

喜欢所有未定义的行为

if (!this)
{
   return NULL;
}

这是等待引爆的炸弹。如果它适用于您当前的编译器，那么您有点幸运，有点不幸！

相同编译器的下一个版本可能会更加激进，并将其视为死代码。由于this 永远不能为空，因此可以“安全地”删除代码。

我觉得你最好去掉它！

Answer 3

如果您有许多 GetLookup 函数返回 NULL，那么您最好修复使用 NULL 指针调用方法的代码。一、替换

if (!this) return NULL;

与

if (!this) {
  // TODO(Crashworks): Replace this case with an assertion on July, 2012, once all callers are fixed.
  printf("Please mail the following stack trace to myemailaddress. Thanks!");
  print_stacktrace();
  return NULL;
}

现在，继续你的其他工作，但在它们滚动时修复它们。替换：

GetLookup(x)->Lookup(y)...

与

convert_to_proxy(GetLookup(x))->Lookup(y)...

conver_to_proxy 确实返回指针不变，除非它是 NULL，在这种情况下，它返回一个 FailedLookupObject，就像我在其他答案中一样。

Answer 4

它在大多数编译器中可能不会崩溃，因为非虚拟函数通常被内联或转换为以“this”作为参数的非成员函数。但是，该标准明确指出，在对象的生命周期之外调用非静态成员函数是未定义的，并且对象的生命周期定义为从对象的内存已分配且构造函数完成时开始，如果它有非平凡的初始化。

该标准仅对对象自身在构造或销毁期间进行的调用对该规则进行了例外处理，但即使如此，也必须小心，因为虚拟调用的行为可能与对象生命周期内的行为不同。

TL:DR：我会用火杀死它，即使清理所有呼叫站点需要很长时间。

Answer 5

编译器的未来版本可能会在正式未定义行为的情况下进行更积极的优化。我不会担心现有的部署（你知道编译器实际实现的行为），但它应该在源代码中修复，以防你使用不同的编译器或不同的版本。

Answer 6

这就是所谓的“聪明而丑陋的黑客”。注意：聪明！= 明智。

在没有任何重构工具的情况下找到所有调用站点应该很容易；以某种方式破坏 GetLookup() 使其无法编译（例如更改签名），因此您可以静态识别误用。然后添加一个名为 DoLookup() 的函数，它可以完成所有这些黑客现在正在做的事情。

Answer 7

在这种情况下，我建议从成员函数中删除 NULL 检查并创建一个非成员函数

CThingy* SafeLookup(CLookupThing *lookupThing) {
  if (lookupThing == NULL) {
    return NULL;
  } else {
    return lookupThing->Lookup();
  }
}

那么应该很容易找到对 Lookup 成员函数的每个调用，并将其替换为安全的非成员函数。

Answer 8

如果今天有什么事情困扰着你，那么一年后它就会困扰着你。正如您所指出的，更改它几乎肯定会引入一些错误 - 但您可以从保留 return NULL 功能开始，添加一些日志记录，让它在野外运行几周，然后找到它的次数甚至被击中？

Answer 9

您现在可以安全地修复此问题，方法是返回失败的查找对象。

class CLookupThingy: public Interface {
  // ...
}

class CFailedLookupThingy: public Interface {
 public:
  CThingy* Lookup(string const& name) {
    return NULL;
  }
  operator bool() const { return false; }  // So that GetLookup() can be tested in a condition.
} failed_lookup;

Interface *GetLookup() {
  if (notReady())
    return &failed_lookup;
  // else etc...
}

这段代码仍然有效：

CThingy *pFoo = GetLookup()->Lookup( "foo" ); // will set pFoo to NULL without crashing

Answer 10

仅当您对规范的措辞持怀疑态度时，这才是“定时炸弹”。然而，无论如何，这是一种糟糕的、不明智的方法，因为它掩盖了程序错误。仅出于这个原因，我会删除它，即使这意味着大量工作。这不是直接（甚至是中期）风险，但也不是一个好方法。

这种错误隐藏行为也确实不是您想要依赖的。想象一下，您依赖于这种行为（即 我的对象是否有效并不重要，它无论如何都会工作！）然后，出于某种危险，编译器优化了 if 语句中的特殊情况，因为它可以证明this 不是空指针。这不仅适用于一些假设的未来编译器，而且适用于非常真实的当前编译器。
但是，当然，由于您的程序格式不正确，发生在某些时候，您会在大约 20 个角处向它传递一个空的 this。砰，你死定了。
这是非常人为的，诚然，它不会发生，但你不能 100% 确定它仍然不可能发生。

请注意，当我喊出“移除！”时，这并不意味着必须立即或在一次大规模的人力操作中移除全部。您可以在遇到这些检查时一一删除（无论如何更改同一文件中的某些内容时，请避免重新编译），或者只是文本搜索一项（最好在高度使用的函数中），然后删除该检查。

由于您使用的是 GCC，您可能会对__builtin_return_address 感兴趣，它可以帮助您在无需大量人力的情况下移除这些检查，并完全打乱整个工作流程并导致应用程序完全无法使用。
之前 删除检查，修改它以输出调用者的地址，addr2line 会告诉你源中的位置。这样，您应该能够快速识别应用程序中行为错误的所有位置，以便修复这些问题。

所以不是

if(!this) return 0;

一次将一个位置更改为：

if(!this) { __builtin_printf("!!! %p\n", __builtin_return_address(0)); return 0; }

这使您可以识别此更改的无效调用站点，同时仍让程序“按预期工作”（如果需要，您还可以查询调用者的调用者）。一个一个地修复每一个行为不端的位置。该程序仍将正常“工作”。
一旦没有更多地址出现，请一起删除检查。如果您不走运，您可能仍然需要修复一个或另一个崩溃（因为它在您测试时没有显示），但这应该是非常罕见的事情发生。无论如何，它应该可以防止你的同事对你大喊大叫。
每周删除一到两张支票，最终不会留下任何支票。与此同时，生活还在继续，根本没有人注意到你在做什么。

TL;DR
至于“当前版本的 GCC”，您可以使用非虚拟功能，但当然没有人知道未来版本可能会做什么。但是，我认为将来的版本极不可能导致您的代码中断。不少现有项目都有这种检查（我记得我们在 Code::Blocks 代码完成中确实有数百个，不要问我为什么！）。编译器制造商可能不想故意让数十/数百个主要项目维护者不高兴，只是为了证明一点。
另外，请考虑最后一段（“从逻辑的角度来看”）。即使这个检查会在未来的编译器中崩溃和烧毁，它仍然会崩溃和烧毁。

if(!this) return; 语句有点没用，因为this 在格式良好的程序中永远不能是空指针（这意味着您在空指针上调用了成员函数）。当然，这并不意味着它不可能发生。但在这种情况下，程序应该严重崩溃或因断言而中止。在任何情况下，这样的程序都不应无声无息地继续。
另一方面，完全有可能在 invalid 对象上调用成员函数，而该对象恰好是 not null。检查this 是否是空指针显然不能捕捉到这种情况，但它是完全相同的UB。因此，除了隐藏错误行为外，此检查还只检测到一半的问题案例。

如果您按照规范的措辞，使用this（包括检查它是否为空指针）是未定义的行为。就目前而言，严格来说，它是一颗“定时炸弹”。但是，无论从实践的角度还是从逻辑的角度来看，我都不会合理地认为这是一个问题。

• 从实用的角度来看，你是否读取一个无效的指针并不重要，只要你不取消引用它。是的，严格来说，这是不允许的。是的，理论上有人可能会构建一个 CPU，它会在您 加载 无效指针时检查它们并出错。唉，事实并非如此，如果你是真实的。
• 从逻辑的角度来看，假设检查会爆炸，它仍然不会发生。要执行此语句，必须调用成员函数，并且（虚拟或非虚拟，内联或非内联）使用无效的this，它在函数体内提供。如果对this 的一次非法使用被炸毁，另一种也会发生。因此，该检查已被废弃，因为该程序已经在之前崩溃了。

---

n.b.：此检查与“安全删除习惯用法”非常相似，后者在删除 nullptr 后将其设置为指针（使用宏或模板化的 safe_delete 函数）。据推测，这是“安全的”，因为它不会崩溃两次删除同一个指针。有些人甚至添加了一个多余的if(!ptr) delete ptr;。
如您所知，operator delete 保证是对空指针的无操作。这意味着通过设置一个指向空指针的指针，您已经成功消除了检测双重删除的唯一机会（这是一个需要修复的程序错误！）。它不是任何“更安全”，而是隐藏了不正确的程序行为。如果您两次删除一个对象，程序应该严重崩溃。
您应该单独保留已删除的指针，或者，如果您坚持篡改，请将其设置为非空无效指针（例如特殊“无效”全局变量的地址，或者像 -1 这样的魔术值，如果您会——但你不应该尝试作弊并在崩溃发生时隐藏）。

Answer 11

我个人认为，你应该尽早失败以提醒你注意问题。在这种情况下，我会毫不客气地删除我能找到的每一个 if(!this)。