Wie schlecht ist "if (! This)" in einer C ++ - Mitgliedsfunktion?

Wenn ich auf alten Code stoße, der if (!this) return;in einer App funktioniert, wie hoch ist das Risiko? Ist es eine gefährliche tickende Zeitbombe, die eine sofortige App-weite Suche und Zerstörung erfordert, oder ist es eher ein Code-Geruch, der ruhig an Ort und Stelle gelassen werden kann?

Ich habe natürlich nicht vor , Code zu schreiben , der dies tut. Vielmehr habe ich kürzlich etwas in einer alten Kernbibliothek entdeckt, die von vielen Teilen unserer App verwendet wird.

Stellen Sie sich vor, eine CLookupThingyKlasse hat eine nicht virtuelle CThingy *CLookupThingy::Lookup( name ) Mitgliedsfunktion. Anscheinend stieß einer der Programmierer in jenen Cowboy-Tagen auf viele Abstürze, bei denen NULL-Werte CLookupThingy *von Funktionen übergeben wurden, und anstatt Hunderte von Anrufseiten zu reparieren, reparierte er leise 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

Ich habe dieses Juwel Anfang dieser Woche entdeckt, bin aber jetzt in Konflikt geraten, ob ich es reparieren sollte. Dies ist eine Kernbibliothek, die von allen unseren Apps verwendet wird. Einige dieser Apps wurden bereits an Millionen von Kunden ausgeliefert, und es scheint gut zu funktionieren. Es gibt keine Abstürze oder andere Fehler von diesem Code. Das Entfernen der if !thisFunktion in der Suche bedeutet, dass Tausende von Anrufstellen repariert werden, die möglicherweise NULL übergeben. unvermeidlich werden einige verpasst haben , die Einführung neuer Fehler , die zufällig über die nächste Pop - up wird Jahr der Entwicklung.

Also neige ich dazu, es in Ruhe zu lassen, es sei denn, dies ist absolut notwendig.

Wie gefährlich ist es if (!this)in der Praxis , da es sich um technisch undefiniertes Verhalten handelt ? Lohnt es sich, Mannwochen Arbeit zu reparieren, oder kann man sich darauf verlassen, dass MSVC und GCC sicher zurückkehren?

Unsere App wird unter MSVC und GCC kompiliert und läuft unter Windows, Ubuntu und MacOS. Die Portabilität auf andere Plattformen spielt keine Rolle. Die betreffende Funktion ist garantiert niemals virtuell.

Bearbeiten: Die Art der objektiven Antwort, die ich suche, ist so etwas wie

• "Aktuelle Versionen von MSVC und GCC verwenden einen ABI, bei dem nicht-virtuelle Mitglieder wirklich statisch mit einem impliziten 'this'-Parameter sind. Daher verzweigen sie sicher in die Funktion, selbst wenn' this 'NULL ist" oder
• "Eine bevorstehende Version von GCC wird den ABI so ändern, dass selbst nicht-virtuelle Funktionen das Laden eines Verzweigungsziels vom Klassenzeiger erfordern" oder
• "Das aktuelle GCC 4.5 hat einen inkonsistenten ABI, bei dem manchmal nicht virtuelle Mitglieder als direkte Verzweigungen mit einem impliziten Parameter und manchmal als Klassenversatz-Funktionszeiger kompiliert werden."

Ersteres bedeutet, dass der Code stinkend ist, aber wahrscheinlich nicht kaputt geht. Der zweite ist etwas, das nach einem Compiler-Upgrade getestet werden muss. Letzteres erfordert sofortiges Handeln auch bei hohen Kosten.

Natürlich ist dies ein latenter Fehler, der darauf wartet, passiert zu werden, aber im Moment geht es mir nur darum, das Risiko für unsere spezifischen Compiler zu verringern.

Ich würde es in Ruhe lassen. Dies könnte eine bewusste Wahl als altmodische Version des SafeNavigationOperator gewesen sein . Wie Sie sagen, würde ich neuen Code nicht empfehlen, aber für vorhandenen Code würde ich ihn in Ruhe lassen. Wenn Sie es am Ende ändern, würde ich sicherstellen, dass alle Aufrufe durch Tests gut abgedeckt sind.

Zum Hinzufügen bearbeiten: Sie können es nur in Debug-Versionen Ihres Codes entfernen über:

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

Auf diese Weise wird der Produktionscode nicht beschädigt, und Sie können ihn im Debug-Modus testen.

Wie alles undefinierte Verhalten

if (!this)
{
   return NULL;
}

Dies ist eine Bombe, die darauf wartet, hochzugehen. Wenn es mit Ihren aktuellen Compilern funktioniert, haben Sie ein bisschen Glück, ein bisschen Pech!

Die nächste Version derselben Compiler ist möglicherweise aggressiver und sieht dies als toten Code an. Da thisniemals null sein kann, kann der Code "sicher" entfernt werden.

Ich denke, es ist besser, wenn Sie es entfernt haben!

Wenn viele GetLookup-Funktionen NULL zurückgeben, sollten Sie Code korrigieren, der Methoden mit einem NULL-Zeiger aufruft. Ersetzen Sie zuerst

if (!this) return NULL;

mit

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

Fahren Sie jetzt mit Ihrer anderen Arbeit fort, aber beheben Sie diese beim Einrollen. Ersetzen Sie:

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

mit

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

Wobei conver_to_proxy den Zeiger unverändert zurückgibt, es sei denn, er ist NULL. In diesem Fall wird ein FailedLookupObject wie in meiner anderen Antwort zurückgegeben.

In den meisten Compilern kann es nicht zum Absturz kommen, da nicht virtuelle Funktionen normalerweise entweder inline oder in Nicht-Member-Funktionen übersetzt werden, wobei "this" als Parameter verwendet wird. Der Standard besagt jedoch ausdrücklich, dass das Aufrufen einer nicht statischen Elementfunktion außerhalb der Lebensdauer des Objekts undefiniert ist und die Lebensdauer eines Objekts als Beginn definiert wird, wenn der Speicher für das Objekt zugewiesen und der Konstruktor abgeschlossen wurde, falls dies der Fall ist nicht triviale Initialisierung.

Der Standard macht nur eine Ausnahme von dieser Regel für Aufrufe, die das Objekt selbst während der Konstruktion oder Zerstörung ausführt. Selbst dann muss man vorsichtig sein, da das Verhalten virtueller Aufrufe vom Verhalten während der Lebensdauer des Objekts abweichen kann.

TL: DR: Ich würde es mit Feuer töten, auch wenn es lange dauern wird, alle Anrufstellen zu bereinigen.

Zukünftige Versionen des Compilers werden bei formal undefiniertem Verhalten wahrscheinlich aggressiver optimieren. Ich würde mich nicht um vorhandene Bereitstellungen kümmern (bei denen Sie das Verhalten kennen, das der Compiler tatsächlich implementiert hat), aber es sollte im Quellcode behoben werden, falls Sie jemals einen anderen Compiler oder eine andere Version verwenden.

Dies ist etwas, das als "kluger und hässlicher Hack" bezeichnet wird. Hinweis: klug! = weise.

Es sollte einfach genug sein, alle Anrufseiten ohne Refactoring-Tools zu finden. Brechen Sie GetLookup () irgendwie ab, damit es nicht kompiliert wird (z. B. Signatur ändern), damit Sie Missbrauch statisch identifizieren können. Fügen Sie dann eine Funktion namens DoLookup () hinzu, die genau das tut, was all diese Hacks gerade tun.

In diesem Fall würde ich vorschlagen, die NULL-Prüfung aus der Member-Funktion zu entfernen und eine Nicht-Member-Funktion zu erstellen

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

Dann sollte es einfach genug sein, jeden Aufruf der LookupMember-Funktion zu finden und durch die sichere Nicht-Member-Funktion zu ersetzen.

Wenn es etwas ist, das Sie heute stört, wird es Sie in einem Jahr stören. Wie Sie bereits betont haben, führt das Ändern mit ziemlicher Sicherheit zu einigen Fehlern. Sie können jedoch zunächst die return NULLFunktionalität beibehalten , ein wenig Protokollierung hinzufügen, einige Wochen in freier Wildbahn laufen lassen und feststellen, wie oft es überhaupt getroffen wird ?

Sie können dies heute sicher beheben, indem Sie ein fehlgeschlagenes Suchobjekt zurückgeben.

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

Dieser Code funktioniert immer noch:

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

Dies ist nur dann eine "tickende Bombe", wenn Sie den Wortlaut der Spezifikation pedantisch beurteilen. Unabhängig davon ist es jedoch ein schrecklicher, schlecht beratener Ansatz, da er einen Programmfehler verdeckt. Allein aus diesem Grund würde ich es entfernen , auch wenn es erhebliche Arbeit bedeutet. Es ist kein unmittelbares (oder sogar mittelfristiges) Risiko, aber es ist einfach kein guter Ansatz.

Ein solches Verhalten beim Ausblenden von Fehlern ist auch nichts, worauf Sie sich verlassen möchten. Stellen Sie sich vor, Sie verlassen sich auf dieses Verhalten (dh es spielt keine Rolle, ob meine Objekte gültig sind, es funktioniert trotzdem! ), Und dann optimiert der Compiler die Gefahr ifin einem bestimmten Fall, weil er beweisen kann, dass dies thiskein a ist Null Zeiger. Dies ist eine legitime Optimierung nicht nur für einen hypothetischen zukünftigen Compiler, sondern auch für sehr reale Compiler der Gegenwart.
Aber natürlich, da Ihr Programm nicht wohlgeformt, es geschieht an einem gewissen Punkt , dass Sie es ein Null passieren thisrund 20 Ecken. Bang, du bist tot.
Das ist zwar sehr verdorben, und es wird nicht passieren, aber Sie können nicht 100% sicher sein, dass es immer noch so istkann unmöglich passieren.

Beachten Sie, dass wenn ich "Entfernen!" Rufe, dies nicht bedeutet, dass die gesamte Menge sofort oder in einem massiven Arbeitskräftebetrieb entfernt werden muss. Sie können diese Überprüfungen einzeln entfernen, wenn Sie auf sie stoßen (wenn Sie ohnehin etwas in derselben Datei ändern, vermeiden Sie Neukompilierungen), oder einfach nach einer Textsuche suchen (vorzugsweise in einer häufig verwendeten Funktion) und diese entfernen.

Da Sie GCC verwenden, sind Sie möglicherweise interessiert __builtin_return_address, was Ihnen dabei helfen kann, diese Überprüfungen ohne großen Personalaufwand zu entfernen, den gesamten Workflow vollständig zu stören und die Anwendung vollständig unbrauchbar zu machen. Ändern Sie die Prüfung
vor dem Entfernen so, dass sie die Adresse des Anrufers ausgibt, und addr2linegeben Sie den Speicherort in Ihrer Quelle an. Auf diese Weise sollten Sie in der Lage sein, schnell alle Speicherorte in der Anwendung zu identifizieren, die sich falsch verhalten, damit Sie diese beheben können.

Also statt

if(!this) return 0;

Ändern Sie jeweils einen Ort in:

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

Auf diese Weise können Sie die ungültigen Anrufstellen für diese Änderung identifizieren, während das Programm weiterhin "wie beabsichtigt funktioniert" (bei Bedarf können Sie auch den Anrufer des Anrufers abfragen). Beheben Sie jeden schlecht benommenen Ort nacheinander. Das Programm "funktioniert" weiterhin wie gewohnt.
Wenn keine Adressen mehr angezeigt werden, entfernen Sie den Scheck insgesamt. Möglicherweise müssen Sie den einen oder anderen Absturz noch beheben, wenn Sie Pech haben (weil er beim Testen nicht angezeigt wurde), aber das sollte sehr selten vorkommen. In jedem Fall sollte es Ihren Kollegen daran hindern, Sie anzuschreien.
Entfernen Sie ein oder zwei Schecks pro Woche, und schließlich bleiben keine übrig. In der Zwischenzeit geht das Leben weiter und niemand merkt, was Sie überhaupt tun.

TL; DR
Was "aktuelle Versionen von GCC" betrifft, sind Sie für nicht virtuelle Funktionen in Ordnung, aber natürlich kann niemand sagen, was eine zukünftige Version tun könnte. Ich halte es jedoch für sehr unwahrscheinlich, dass eine zukünftige Version dazu führt, dass Ihr Code beschädigt wird. Nicht wenige bestehende Projekte haben diese Art der Überprüfung (ich erinnere mich, dass wir buchstäblich Hunderte davon in Code :: Blocks Code-Vervollständigung hatten, fragen Sie mich nicht warum!). Compilerhersteller möchten wahrscheinlich nicht Dutzende / Hunderte von großen Projektbetreuern absichtlich unglücklich machen, nur um einen Punkt zu beweisen.
Beachten Sie auch den letzten Absatz ("aus logischer Sicht"). Selbst wenn diese Prüfung mit einem zukünftigen Compiler abstürzt und brennt, stürzt sie trotzdem ab und brennt.

Die if(!this) return;Anweisung ist insofern etwas nutzlos, als sie thisin einem wohlgeformten Programm niemals ein Nullzeiger sein kann (dies bedeutet, dass Sie eine Mitgliedsfunktion für einen Nullzeiger aufgerufen haben). Dies bedeutet natürlich nicht, dass es unmöglich passieren könnte. In diesem Fall sollte das Programm jedoch stark abstürzen oder mit einer Behauptung abbrechen. Ein solches Programm sollte unter keinen Umständen stillschweigend fortgesetzt werden.
Andererseits ist es durchaus möglich, eine Mitgliedsfunktion für ein ungültiges Objekt aufzurufen , das zufällig nicht null ist . Das Überprüfen, ob thises sich um den Nullzeiger handelt, fängt diesen Fall offensichtlich nicht ab, aber es ist genau das gleiche UB. Abgesehen davon, dass falsches Verhalten verborgen ist, erkennt diese Überprüfung auch nur die Hälfte der problematischen Fälle.

Wenn Sie sich an den Wortlaut der Spezifikation halten, ist die Verwendung von this(einschließlich der Überprüfung, ob es sich um einen Nullzeiger handelt) ein undefiniertes Verhalten. Insofern handelt es sich streng genommen um eine "Zeitbombe". Ich würde dies jedoch nicht als vernünftig erachten, sowohl aus praktischer als auch aus logischer Sicht.

• Aus praktischer Sicht spielt es keine Rolle, ob Sie einen ungültigen Zeiger lesen , solange Sie ihn nicht dereferenzieren . Ja, streng genommen ist dies nicht erlaubt. Ja, theoretisch könnte jemand eine CPU bauen, die beim Laden ungültige Zeiger und Fehler überprüft . Leider ist dies nicht der Fall, wenn Sie real sind.
• Von einem logischen Standpunkt aus, unter der Annahme , dass der Scheck wird die Luft zu sprengen, wird es immer noch nicht geschehen. Damit diese Anweisung ausgeführt werden kann, muss die Member-Funktion aufgerufen werden und verwendet (virtuell oder nicht, inline oder nicht) einen ungültigen Wert this, den sie im Funktionskörper zur Verfügung stellt. Wenn ein illegitimer Gebrauch thisexplodiert, wird es auch der andere tun. Daher ist die Prüfung veraltet, da das Programm bereits früher abstürzt.

---

nb: Diese Prüfung ist der "sicheren Löschsprache" sehr ähnlich, auf die nullptrnach dem Löschen ein Zeiger gesetzt wird (mithilfe eines Makros oder einer Vorlagenfunktion safe_delete). Vermutlich ist dies "sicher", da es nicht abstürzt, wenn derselbe Zeiger zweimal gelöscht wird. Einige Leute fügen sogar eine überflüssige hinzu if(!ptr) delete ptr;.
Wie Sie wissen, operator deleteist dies garantiert ein No-Op für einen Nullzeiger. Dies bedeutet nicht mehr und nicht weniger als durch Setzen eines Zeigers auf den Nullzeiger. Sie haben die einzige Möglichkeit, eine doppelte Löschung zu erkennen, erfolgreich ausgeschlossen (dies ist ein Programmfehler, der behoben werden muss!). Es ist nicht "sicherer", sondern verbirgt falsches Programmverhalten. Wenn Sie ein Objekt zweimal löschen, das Programm sollte hart abstürzen.
Sie sollten entweder einen gelöschten Zeiger in Ruhe lassen oder, wenn Sie auf Manipulationen bestehen, ihn auf einen ungültigen Zeiger ungleich Null setzen (z. B. die Adresse einer speziellen "ungültigen" globalen Variablen oder einen magischen Wert, wie -1wenn Sie wollen - Sie sollten jedoch nicht versuchen, den Absturz zu betrügen und zu verbergen, wenn er auftreten soll.

Ich persönlich bin der Meinung, dass Sie so früh wie möglich versagen sollten, um Sie auf Probleme aufmerksam zu machen. In diesem Fall würde ich jedes einzelne Vorkommen, das if(!this)ich finden konnte, kurzerhand entfernen .