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

Question

如果我遇到if (!this) return;应用程序中的旧代码,那么风险有多严重？这是一个危险的滴答作响的定时炸弹需要立即在应用程序范围内搜索并摧毁努力,还是更像是可以安静地留在原点的代码气味？

当然,我不打算编写执行此操作的代码.相反,我最近在我们的应用程序的许多部分使用的旧核心库中发现了一些东西.

想象一下,一个CLookupThingy类具有非虚拟 CThingy *CLookupThingy::Lookup( name )成员函数.显然,那些牛仔时代的程序员遇到了许多崩溃事件,其中NULL CLookupThingy *是从函数传递的,而不是修复数百个调用站点,他悄悄地修复了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永远不会为null,因此可以"安全地"删除代码.

我认为如果你删除它会更好!

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成员函数的每次调用,并用安全的非成员函数替换它.