Comment le mauvais est "if (!c')" dans une fonction de membre C++?

Question

Si je tombe sur un vieux code qui n' if (!this) return; dans une application, la gravité du risque est-ce? Est-ce une dangereuse bombe à retardement qui nécessite une application immédiate de recherche à l'échelle et de détruire l'effort, ou est-il plus comme une odeur de code qui peuvent être parti tranquillement en place?

Je ne suis pas la planification sur l'écriture de code qui fait cela, bien sûr. Plutôt, j'ai récemment découvert quelque chose dans une vieille bibliothèque de base utilisé par de nombreuses pièces de notre application.

Imaginez un CLookupThingy classe a un non-virtuel, CThingy *CLookupThingy::Lookup( name ) de la fonction membre. Apparemment, l'un des programmeurs de retour dans les cowboy jours a rencontré de nombreux plantages où NULL CLookupThingy *s ont été transmis à partir de fonctions, et plutôt que de fixer des centaines de sites d'appel, tranquillement, il fixe jusqu'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

J'ai découvert ce petit bijou plus tôt cette semaine, mais maintenant je suis en conflit quant à savoir si je devrais le réparer. C'est dans une bibliothèque de base utilisé par tous de nos applications. Plusieurs de ces applications ont déjà été livrés à des millions de clients, et il semble fonctionner correctement; il n'y a pas des accidents ou d'autres insectes à partir de ce code. Retrait de l' if !this dans la fonction recherche signifie la fixation des milliers de sites d'appel qui, potentiellement, passer la valeur NULL; inévitablement, ne sera pas atteint, l'introduction de nouveaux bugs qui apparaîtront de façon aléatoire au cours de la prochaine année de développement.

De sorte que je suis enclin à le laisser seul, sauf si absolument nécessaire.

Étant donné qu'il est techniquement un comportement indéfini, quel est le danger d' if (!this) dans la pratique? Est-il digne de l'homme-semaines de travail à fixer, ou peut MSVC et GCC compter sur le retour en toute sécurité?

Notre application compile sur MSVC et de GCC, et fonctionne sur Windows, Ubuntu, et MacOS. Transférabilité à d'autres plates-formes n'est pas pertinent. La fonction en question est la garantie de ne jamais être virtuel.

Edit: Le genre de réponse objective, je suis à la recherche de quelque chose comme

• "Les versions actuelles de MSVC et GCC utilisation de l'ABI où nonvirtual les membres sont vraiment statique avec un implicite de ce paramètre; c'est pourquoi ils seront en toute sécurité dans la fonction, même si" il "est NULL" ou
• "une prochaine version de GCC va changer l'ABI de sorte que même nonvirtual fonctions nécessitent le chargement d'une direction de la cible à partir du pointeur de classe" ou
• "la GCC 4.5 a une incohérence ABI où, parfois, il compile nonvirtual membres de succursales directes avec un paramètre implicite, et parfois en tant que classe-décalage des pointeurs de fonction."

L'ancien signifie que le code est puant, mais peu probable de pause; le second est quelque chose à tester après un compilateur de mise à niveau; celui-ci exige une action immédiate, même à coût élevé.

C'est clairement latente bug en attente de se produire, mais pour l'instant je me préoccupe seulement de réduire les risques sur nos compilateurs.

Answer 1

Je voudrais le laisser seul. Ce pourrait avoir été un choix délibéré que l'ancienne version de la SafeNavigationOperator. Comme vous le dites, dans le nouveau code, je ne le recommande pas, mais pour le code existant, je préfère le laisser seul. Si vous ne finissent par modifier, je serais assurez-vous que tous les appels sont bien couverts par des tests.

Modifier pour ajouter: vous pourriez choisir de le supprimer uniquement dans les versions de débogage de votre code par:

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

Ainsi, il ne serait pas casser quoi que ce soit sur le code de production, tout en vous donnant une chance de le tester en mode de débogage.

Answer 2

Comme tout comportement non défini

if (!this)
{
   return NULL;
}

c' est une bombe d'attente pour aller au large. Si cela fonctionne avec votre compilateurs gcc, vous êtes le genre de la chance, une sorte de pas de chance!

La prochaine version de la même compilateurs pourrait être plus agressif et voir ce que le code mort. En tant que this ne peut jamais être nulle, le code peut "en toute sécurité" soient supprimés.

Je pense que c'est mieux si vous l'avez supprimé!

Answer 3

Si vous avez de nombreux GetLookup fonctions renvoient la valeur NULL, alors vous êtes mieux de fixation code qui appelle les méthodes à l'aide d'un pointeur NULL. Tout d'abord, remplacer

if (!this) return NULL;

avec

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;
}

Maintenant, continuez votre autre travail, mais à corriger ce qu'ils roulent. Remplacer:

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

avec

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

Où conver_to_proxy ne retourne le pointeur inchangé, sauf s'il est NULL, auquel cas il renvoie une FailedLookupObject comme dans mon autre réponse.

Answer 4

Il peut ne pas planter dans la plupart des compilateurs, puisque d'autres fonctions virtuelles sont généralement soit incorporé ou traduite dans des fonctions de membre de la prise de "cela" comme paramètre. Toutefois, la norme indique expressément que l'appel à un non-membre statique de la fonction à l'extérieur de la durée de vie de l'objet n'est pas définie, et la durée de vie d'un objet est défini comme début, quand la mémoire pour l'objet a été alloué et le constructeur a terminé, si elle est non-trivial d'initialisation.

La norme ne fait exception à cette règle pour les appels effectués par l'objet lui-même lors de la construction ou de la destruction, mais même alors, il faut être prudent, car le comportement de virtuel les appels peuvent différer du comportement au cours de l'objet de la durée de vie.

TL:DR: j'avais tuer avec le feu, même si cela prendra beaucoup de temps pour nettoyer tous les sites d'appel.

Answer 5

Les futures versions du compilateur sont probablement plus agressive de les optimiser en cas de officiellement un comportement indéfini. Je ne voudrais pas vous soucier de les déploiements existants (où vous savez que le comportement du compilateur effectivement mis en œuvre), mais il doit être fixé dans le code source dans le cas où vous jamais utiliser un autre compilateur ou l'autre version.