c++ - latest - oop patterns

¿Qué tan malo es "if(! This)" en una función miembro de C++?

Question

Si encuentro un código antiguo que hace if (!this) return; en una aplicación, ¿qué tan grave es este riesgo? ¿Es una bomba de relojería peligrosa que requiere una búsqueda inmediata en toda la aplicación y destruir el esfuerzo, o es más como un olor a código que puede dejarse en silencio?

No estoy planeando escribir código que haga esto, por supuesto. Más bien, recientemente descubrí algo en una antigua biblioteca central utilizada por muchos elementos de nuestra aplicación.

Imagine que una clase CLookupThingy tiene una función de miembro CThingy *CLookupThingy::Lookup( name ) no virtual . Aparentemente, uno de los programadores en esos días de cowboy se encontró con muchos bloqueos en los que se CLookupThingy * NULL CLookupThingy * s de las funciones, y en lugar de arreglar cientos de sitios de llamadas, silenciosamente arregló 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

Descubrí esta gema a principios de esta semana, pero ahora tengo un conflicto sobre si debo arreglarlo. Esto se encuentra en una biblioteca principal utilizada por todas nuestras aplicaciones. Varias de esas aplicaciones ya han sido enviadas a millones de clientes, y parece estar funcionando bien; no hay bloqueos u otros errores de ese código. Quitar el if !this en la función de búsqueda significará reparar miles de sitios de llamadas que potencialmente pasan NULL; inevitablemente algunos se perderán, introduciendo nuevos errores que aparecerán aleatoriamente durante el próximo año de desarrollo.

Así que estoy dispuesto a dejarlo solo, a menos que sea absolutamente necesario.

Dado que se trata de un comportamiento técnicamente indefinido, ¿qué tan peligroso es if (!this) en la práctica? ¿Vale la pena arreglarlo con una semana hombre de trabajo, o se puede contar con MSVC y GCC para regresar de manera segura?

Nuestra aplicación se compila en MSVC y GCC, y se ejecuta en Windows, Ubuntu y MacOS. La portabilidad a otras plataformas es irrelevante. La función en cuestión está garantizada para nunca ser virtual.

Editar: El tipo de respuesta objetiva que estoy buscando es algo así como

• "Las versiones actuales de MSVC y GCC usan un ABI donde los miembros no virtuales son realmente estáticos con un parámetro ''this'' implícito, por lo que se ramificarán de forma segura en la función incluso si ''this'' es NULL" o
• "una próxima versión de GCC cambiará el ABI de modo que incluso las funciones no virtuales requieren cargar un destino de rama desde el puntero de clase" o
• "el GCC 4.5 actual tiene un ABI inconsistente donde a veces compila miembros no virtuales como ramas directas con un parámetro implícito, y algunas veces como punteros de funciones de desplazamiento de clase".

El primero significa que el código es apestoso pero es poco probable que se rompa; el segundo es algo para probar después de una actualización del compilador; este último requiere acción inmediata incluso a un alto costo.

Claramente, este es un error latente que está por suceder, pero ahora solo me preocupa mitigar el riesgo en nuestros compiladores específicos.

Answer 1

Como todo comportamiento indefinido

if (!this)
{
   return NULL;
}

esta es una bomba esperando estallar. Si funciona con tus compiladores actuales, ¡eres amable, afortunado, amable, desafortunado!

La próxima versión de los mismos compiladores podría ser más agresiva y ver esto como un código muerto. Como this nunca puede ser nulo, el código puede eliminarse "de manera segura".

¡Creo que es mejor si lo eliminaste!

Answer 2

En este caso, sugeriría eliminar la verificación NULL de la función miembro y crear una función no miembro

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

Entonces debería ser lo suficientemente fácil encontrar cada llamada a la función de miembro Lookup y reemplazarla con la función safe non-member.

Answer 3

En mi opinión personal, debes fallar lo antes posible para avisarte de los problemas. En ese caso, eliminaría sin miramientos todas y cada una de las apariciones de if(!this) que pudiera encontrar.

Answer 4

Es posible que no se cuelgue en la mayoría de los compiladores, ya que las funciones no virtuales suelen estar en línea o traducidas a funciones que no son miembros, tomando "esto" como parámetro. Sin embargo, el estándar dice específicamente que llamar a una función miembro no estática fuera de la vida útil del objeto no está definido, y la duración de un objeto se define como el principio cuando se asignó la memoria para el objeto y el constructor se completó, si inicialización no trivial.

El estándar solo hace una excepción a esta regla para las llamadas hechas por el objeto mismo durante la construcción o destrucción, pero incluso entonces hay que tener cuidado porque el comportamiento de las llamadas virtuales puede diferir del comportamiento durante la vida del objeto.

TL: DR: Lo mataría con fuego, incluso si llevara mucho tiempo limpiar todos los sitios de llamadas.

Answer 5

Es probable que las versiones futuras del compilador optimicen más agresivamente en casos de comportamiento formalmente indefinido. No me preocuparía por las implementaciones existentes (donde se conoce el comportamiento que el compilador realmente implementó), pero debería corregirse en el código fuente en caso de que alguna vez use un compilador diferente o una versión diferente.

Answer 6

Esta es una "bomba de tiempo" solo si eres pedante sobre la redacción de la especificación. Sin embargo, independientemente de eso, es un enfoque terrible y desacertado porque oculta un error del programa. Por esa sola razón, lo eliminaría , incluso si significa un trabajo considerable. No es un riesgo inmediato (ni siquiera a mediano plazo), pero simplemente no es un buen enfoque.

Tal comportamiento de ocultamiento de errores tampoco es algo en lo que quieras confiar. Imagine que confía en este comportamiento (es decir , no importa si mis objetos son válidos, ¡funcionará de todos modos! ) Y luego, por algún peligro, el compilador optimiza la declaración if en un caso particular porque puede probar que this es no es un puntero nulo Esa es una optimización legítima no solo para un futuro compilador hipotético, sino también para compiladores reales muy actuales.
Pero, por supuesto, dado que su programa no está bien formado, sucede que en algún momento lo pasa nulo en 20 esquinas. Bang, estás muerto.
Eso es muy complicado, sin duda, y no sucederá, pero no puedes estar 100% seguro de que todavía no puede suceder.

Tenga en cuenta que cuando grito "¡Eliminar!", Eso no quiere decir que todos ellos deben ser eliminados inmediatamente o en una operación masiva de mano de obra. Puede eliminar estas comprobaciones una por una a medida que las encuentre (cuando cambie algo en el mismo archivo de todos modos, evite las recompilaciones), o simplemente busque texto por una (preferiblemente en una función muy utilizada), y elimine esa.

Como está utilizando GCC, puede estar __builtin_return_address en __builtin_return_address , lo que puede ayudarlo a eliminar estas comprobaciones sin tener que __builtin_return_address mano de obra masiva, interrumpir totalmente el flujo de trabajo y dejar la aplicación completamente inutilizable.
Antes de quitar la verificación, addr2line para dar salida a la dirección de la persona que llama, y addr2line le dirá la ubicación en su fuente. De esta forma, debería poder identificar rápidamente todas las ubicaciones de la aplicación que se comportan incorrectamente, para que pueda solucionarlas.

Entonces, en lugar de

if(!this) return 0;

cambiar una ubicación a la vez a algo como:

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

Eso le permite identificar los sitios de llamadas no válidas para este cambio al mismo tiempo que permite que el programa "funcione como se esperaba" (también puede consultar la persona que llama si es necesario). Repara todas las ubicaciones de mal comportamiento, una por una. El programa seguirá "funcionando" como siempre.
Una vez que no aparezcan más direcciones, elimine el cheque por completo. Es posible que aún tengas que arreglar uno o el otro choque si no tienes suerte (porque no se mostró mientras probabas), pero eso debería ser algo muy raro que ocurra. En cualquier caso, debe evitar que su compañero de trabajo le grite.
Elimine uno o dos cheques por semana y, finalmente, no quedará ninguno. Mientras tanto, la vida continúa y nadie se da cuenta de lo que estás haciendo.

TL; DR
En cuanto a las "versiones actuales de GCC", está bien para las funciones no virtuales, pero por supuesto nadie puede decir lo que una versión futura podría hacer. Sin embargo, considero que es muy poco probable que una versión futura haga que su código se rompa. No pocos proyectos existentes tienen este tipo de control (recuerdo que literalmente había cientos de ellos en el código Code Complete completado en algún momento, ¡no me pregunten por qué!). Es probable que los fabricantes de compiladores no quieran hacer docenas / cientos de desarrolladores de proyectos principales infelices a propósito, solo para demostrar un punto.
Además, considere el último párrafo ("desde un punto de vista lógico"). Incluso si esta comprobación se bloquea y se quema con un compilador futuro, se bloqueará y se quemará de todos modos.

El if(!this) return; declaración es algo inútil en la medida en que this nunca puede ser un puntero nulo en un programa bien formado (significa que llamó a una función miembro en un puntero nulo). Esto no significa, por supuesto, que posiblemente no podría suceder. Pero en este caso, el programa debería bloquearse o abortarse con una afirmación. Bajo ninguna circunstancia debe tal programa continuar silenciosamente.
Por otro lado, es perfectamente posible llamar a una función miembro en un objeto no válido que no es nulo . Comprobar si this es el puntero nulo obviamente no capta ese caso, pero es exactamente el mismo UB. Entonces, además de ocultar el comportamiento incorrecto, esta verificación también solo detecta la mitad de los casos problemáticos.

Si te vas por la redacción de la especificación, usar this (que incluye verificar si es un puntero nulo) es un comportamiento indefinido. Hasta el momento, estrictamente hablando, es una "bomba de tiempo". Sin embargo, no lo consideraría razonablemente como un problema, tanto desde el punto de vista práctico como lógico.

• Desde un punto de vista práctico , realmente no importa si lee un puntero que no es válido siempre y cuando no lo desreferencia . Sí, estrictamente al pie de la letra, esto no está permitido. Sí, en teoría, alguien podría construir una CPU que verificará los punteros no válidos cuando los cargue , y la falla. Por desgracia, este no es el caso, si estás siendo real.
• Desde un punto de vista lógico , suponiendo que la verificación explote, todavía no va a suceder. Para que se ejecute esta instrucción, se debe invocar a la función miembro, y (virtual o no, en línea o no) está utilizando una función no válida, que está disponible dentro del cuerpo de la función. Si un uso ilegítimo de this explota, el otro también lo hará. Por lo tanto, el cheque se está quedando obsoleto porque el programa ya se bloquea antes.

nb: Esta comprobación es muy similar a la "expresión de eliminación segura" que establece un puntero a nullptr después de eliminarlo (utilizando una macro o una función safe_delete plantilla). Presumiblemente, esto es "seguro" porque no falla al eliminar el mismo puntero dos veces. Algunas personas incluso agregan un if(!ptr) delete ptr; .
Como sabe, la operator delete garantiza que no se utilizará un puntero nulo. Lo que significa nada más y nada menos que establecer un puntero al puntero nulo, ha eliminado con éxito la única posibilidad de detectar la eliminación doble (¡que es un error del programa que debe corregirse!). No es "más seguro", pero oculta el comportamiento incorrecto del programa. Si elimina un objeto dos veces, el programa debería bloquearse.
Debe dejar un puntero eliminado solo o, si insiste en alterarlo, configúrelo en un puntero no válido nulo (como la dirección de una variable global especial "no válida" o un valor mágico como -1 si lo desea). - pero no deberías tratar de engañar y ocultar el choque cuando se produzca).

Answer 7

Me gustaría dejarlo solo. Esta podría haber sido una elección deliberada como una versión anticuada de SafeNavigationOperator . Como dices, en un código nuevo, no lo recomendaría, pero para el código existente, lo dejaría en paz. Si terminas modificándolo, me aseguraré de que todas las llamadas estén bien cubiertas por las pruebas.

Editar para agregar: puede elegir eliminarlo solo en versiones de depuración de su código a través de:

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

Por lo tanto, no rompería nada en el código de producción, al tiempo que le da la oportunidad de probarlo en modo de depuración.

Answer 8

Puede solucionarlo de forma segura hoy devolviendo un objeto de búsqueda fallido.

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

Este código todavía funciona:

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

Answer 9

Si es algo que te molesta hoy, te molestará dentro de un año. Como usted señaló, cambiarlo casi seguramente introducirá algunos errores, pero puede comenzar reteniendo la funcionalidad de return NULL , agregar un poco de registro, dejar que se ejecute en la naturaleza durante algunas semanas, y encontrar cuántas veces incluso ¿Es golpeado?

Answer 10

Si tiene muchas funciones de GetLookup que devuelven NULL, entonces es mejor que arregle el código que llama a los métodos usando un puntero NULL. Primero, reemplace

if (!this) return NULL;

con

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

Ahora, continúe con su otro trabajo, pero corrija esto a medida que avanza. Reemplace:

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

con

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

Donde conver_to_proxy no devuelve el puntero sin cambios, a menos que sea NULL, en cuyo caso devuelve un FailedLookupObject como en mi otra respuesta.

Answer 11

esto es algo que se llama ''un hack inteligente y feo''. nota: inteligente! = sabio.

encontrar todos los sitios de llamadas sin herramientas de refactorización debería ser lo suficientemente fácil; rompa GetLookup () de alguna manera para que no se compile (por ejemplo, cambie la firma) para que pueda identificar el uso incorrecto de forma estática. luego agrega una función llamada DoLookup () que hace lo que están haciendo todos estos hacks en este momento.