Öffnen, schreiben und schließen Sie eine Datei

7573
dreamlax

Ich habe eine kleine 10-Liner-Funktion, die einige Daten in eine Datei schreibt std::ofstream. Ich habe .close()am Ende meiner Funktion nicht explizit aufgerufen, aber die Codeüberprüfung ist fehlgeschlagen, mit dem Grund, dass es besser ist, sie explizit aus Stil- und Ausführlichkeitsgründen aufzurufen. Ich verstehe, dass es nicht schaden kann, .close()explizit anzurufen, aber wenn man es explizit kurz vor einer returnAussage aufruft, deutet dies auf mangelndes Verständnis oder Vertrauen in RAII hin?

Der C ++ - Standard sagt:

27.8.1.2

virtual ~ basic_filebuf ();

[3] Effekte: Zerstört ein Objekt von class basic_filebuf<charT,traits>. Anrufe close().

Bin ich berechtigt zu argumentieren, dass das Aufrufen .close()am Ende einer Funktion überflüssig und / oder unnötig ist?

bool SomeClass::saveData()
{
    std::ofstream saveFile(m_filename);

    if (!saveFile.is_open())
        return false;

    saveFile << m_member1 << std::endl;
    saveFile << m_member2 << std::endl;

    saveFile.close(); // passed review only with this line
    return true;
}

Die Funktion soll nur zurückkehren, falsewenn die Datei nicht zum Schreiben geöffnet werden konnte.

Antworten
65
Wenn die Prüfer eine Bestätigung benötigen, dass close automatisch aufgerufen wird, ist C ++ wahrscheinlich nicht die beste Sprachauswahl. Der Grund der "Ausführlichkeit" ist besonders besorgniserregend. Fred Nurk vor 10 Jahren 7
"Es ist fehlgeschlagene Code-Überprüfung mit dem Grund, dass es besser ist, es explizit aus ... * Ausführlichkeitsgründen * zu nennen"? â € “Bitte erkläre das letzte Stück, es macht keinen Sinn. Konrad Rudolph vor 10 Jahren 2
Offensichtlich steht der Stil Ihrer Rezensenten mit dem _standard_-Design der _standard_-Bibliothek in Konflikt. kizzx2 vor 10 Jahren 0
@Konrad: Durch Ausführlichkeit meine ich, dass wir die Datei schließen, obwohl sie wissen, dass sie im Destruktor sowieso geschlossen wird, weil sie zeigt, dass wir damit fertig sind. In einigen Fällen haben wir Dateien für lange Zeit geöffnet. Wir haben also die Regel, dass wir jeden Stream explizit schließen. dreamlax vor 10 Jahren 3
Wurde diese Anforderung hinzugefügt, weil * die andere Instanz * einen Fehler verursacht hatte, der von der Codeüberprüfung nicht erkannt wurde? rwong vor 10 Jahren 0
@dreamlax: aber wie ist verbosity überhaupt ein * vorteil *? Ich bin damit einverstanden, dass * explicitness * sein kann und dass dies manchmal Verbosität mit sich bringt - aber dies ist immer ein Kompromiss zwischen den beiden. Ich habe noch nie eine Situation gesehen, in der die Ausführlichkeit an sich schon von Vorteil wäre. Das meinte ich mit "es ergibt keinen Sinn". Konrad Rudolph vor 10 Jahren 1
@rwong: Das könnte ich mir vorstellen. Die Frage ist, warum bleibt die Variable, wenn der Stream nicht mehr benötigt wird? Der Umfang der Variablen sollte meiner Meinung nach begrenzt sein, um mehr oder weniger den Zeitrahmen einer geöffneten Datei darzustellen. Wenn die Stream-Variable noch aktiv ist, deutet dies wahrscheinlich eher darauf hin, dass die Funktion viel zu lang ist. dreamlax vor 10 Jahren 0
@Konrad: Ich schätze, in manchen Situationen ist es besser, "die Dinge vollständig zu buchstabieren", als prägnantere Versionen zu verwenden. Zum Beispiel sind ein paar Zeilen schön geschriebener Code oft besser als ein cleverer, aber esoterischer Einzeiler. Die Regel ist jedoch in diesem speziellen Fall nicht so zutreffend, und ich denke stattdessen, dass der eigentliche Grund für die Exklusivität ist. dreamlax vor 10 Jahren 0
@dreamlax: "weil es zeigt, dass wir wissen, dass wir damit fertig sind" .. Ich denke, das kann ein guter Grund sein. Es zwingt Sie auch, darüber nachzudenken, wann Sie damit fertig sind. Vielleicht ist es meistens kein Problem, aber manchmal ist es wichtig. Was ist Ihre Branche? Ich stimme mit dem Kommentator überein, der sagte, dass diese Frage auf SO besser wäre. n1ckp vor 10 Jahren 0
Ich [änderte den Titel] (/ q / 540 / Revisionen), so dass es beschreibt, was der Code gemäß den [Website-Zielen] tut (/ questions / how-to-ask): "* Geben Sie an, was Ihr Code in Ihrem Titel bewirkt, nicht Ihre Hauptanliegen. * ". Fühlen Sie sich frei, ihm einen anderen Titel zu geben, wenn es etwas passenderes gibt. Toby Speight vor 2 Jahren 0
Ich glaube, Ihre Kritiker sind falsch, aber nicht aus dem Grund, den Sie denken. Wenn der Rückgabewert für den Erfolg "true" sein soll, sollte "false" zurückgegeben werden, wenn * any * der Dateioperationen fehlschlägt, nicht nur der Konstruktor. Ich habe entsprechend geantwortet. Toby Speight vor 2 Jahren 0

8 Antworten auf die Frage

67
Martin York

Ich würde das genaue Gegenteil behaupten.

Einen Stream explizit zu schließen, ist wahrscheinlich nicht das, was Sie tun möchten. Dies liegt daran, dass close()beim Stream der Stream möglicherweise Ausnahmen ausgelöst wird. Wenn Sie also explizit einen Dateistream schließen, ist dies ein Hinweis darauf, dass Sie den Stream schließen und explizit alle Fehler behandeln möchten, die sich aus dem Schließen des Streams ergeben können (Ausnahmen oder fehlerhafte Bits) (oder möglicherweise sagen Sie, wenn dies fehlschlägt wollen schnell fehlschlagen (Ausnahme, die die Anwendung beenden kann)).

Wenn Sie sich nicht für die Fehler interessieren (dh Sie werden sie sowieso nicht behandeln). Sie sollten nur den Destruktor abschließen lassen. Dies liegt daran, dass der Destruktor alle Ausnahmen abfängt und verwirft, wodurch der Code normal fließen kann. Wenn Sie sich mit dem Schließen einer Datei beschäftigen, möchten Sie dies normalerweise tun (wenn das Schließen fehlschlägt, ist das wichtig?).

"Wenn das Schließen fehlschlägt, ist es wichtig?" Ich denke, für Ausgabeströme ist dies wahrscheinlich der Fall, da die Ausgabe möglicherweise bis zum Aufruf von close () zwischengespeichert wird. Roddy vor 10 Jahren 2
@Roddy: Aber Sie können nichts dagegen unternehmen, um das Scheitern zu verhindern. Es kann wichtig sein, diesen Fehler zu melden, es können jedoch keine anderen sinnvollen Maßnahmen ergriffen werden. Konrad Rudolph vor 10 Jahren 2
@Konrad. Stimmen Sie zu, dass Sie es nicht verhindern können, dass es versagt (aber das stimmt mit vielen Ausnahmen). Wenn der Benutzer "Speichern ..." in einer GUI-App auswählt und close () wirft, ist es wahrscheinlich sehr bedeutsam, den Benutzer zu benachrichtigen und ihm zu erlauben, das Speichern (möglicherweise auf einem anderen Volume) erneut zu versuchen. Roddy vor 10 Jahren 2
@Roddy: gewährt. Aber etwas anderes: tritt dieser Fehler tatsächlich auf? Ich kann mich nicht erinnern, jemals so etwas gesehen zu haben (außer wenn Sie ein USB-Laufwerk vorzeitig abschließen usw.). Konrad Rudolph vor 10 Jahren 0
@Konrad. Nun, ich vermute, Hardwareprobleme, Dateisystembeschädigungen oder Netzwerkprobleme, da dies die einzigen offensichtlichen Probleme sind. Ist das nicht auch beim Schreiben in einen Fstream der Fall? Ich würde noch überlegen, was beim Schreiben mit Ausnahmen passieren würde, selbst wenn ich mich entscheide, sie nicht zu fangen. Roddy vor 10 Jahren 0
@Konrad Ich denke, deshalb heißen sie _Exceptions_ kizzx2 vor 10 Jahren 0
@kizzx: Nein, ist es nicht. Was hat das mit irgendwas zu tun? Konrad Rudolph vor 10 Jahren 0
@Konrad: Ich dachte, du würdest auf "Wenn der Benutzer" Speichern ... "in einer GUI-App auswählt, und close () wirft," Ich habe gerade angenommen "Werfen" bedeutet "wirft Ausnahme", nein? kizzx2 vor 10 Jahren 0
@ kizz: ja. Na und? Konrad Rudolph vor 10 Jahren 0
@Roddy: Natürlich gibt es Zeiten, in denen es eine Rolle spielt und Sie möchten es melden (GUI-Anwendungen). dann würden Sie close () explizit verwenden und den Fehler überprüfen. Ob das Wetter Ihnen wichtig ist oder nicht, ist völlig situativ und nur der Entwickler kann dies am Verwendungsort entscheiden. Im obigen Beispiel hört sich das nicht so an, als ob eine Überprüfung durchgeführt wurde, und deshalb war es ihnen egal. Martin York vor 10 Jahren 2
37
Jerry Coffin

Angenommen, das Fstream-Objekt ist lokal für die Funktion, würde ich dagegen argumentieren. Die Menschen müssen sich daran gewöhnen, dass RAII ihre Arbeit erledigt, und das Schließen eines Fstream-Objekts fällt unter diese Überschrift. Zusätzlicher Code, der nicht sinnvoll ist, ist fast immer eine schlechte Idee.

Edit: Damit ich nicht missverstanden werde, würde ich dagegen nicht nur für diesen speziellen Fall, sondern im Allgemeinen argumentieren. Es ist nicht nur nutzlos, sondern verdunkelt, was nötig ist, und (am aller schlimmsten) ist es auf keinen Fall durchzusetzen - Menschen, die nur in Bezug auf den "normalen" Austritt aus der Funktion denken, müssen das wirklich stoppen In dem Moment, in dem sie Ausnahmebehandlung zu C ++ hinzufügten, änderten sich die Regeln grundlegend. Sie müssen in Bezug auf RAII (oder ähnliches) denken, das die Bereinigung beim Beenden des Bereichs gewährleistet - und das explizite Schließen von Dateien, das Freigeben von Speicher usw. ist nicht zulässig.

Entschuldigung, Ihre Annahme ist richtig, ich habe vergessen zu erwähnen, dass der Fstream lokal für die Funktion ist. dreamlax vor 10 Jahren 0
Ich möchte ein wenig fragen, "was ist der Sinn von Destruktoren, wenn wir die Arbeit manuell erledigen ...", aber ich weiß, dass es nicht weit gehen wird. Ich habe den expliziten Aufruf nur gestellt, um die Codeüberprüfung zu befriedigen, aber ich fühlte mich äußerst zurückhaltend. dreamlax vor 10 Jahren 0
@dreamlax: Es gibt Ihnen Flexibilität. In den meisten Fällen ist es Ihnen egal, ob close () funktioniert oder nicht (Sie können nichts dagegen tun (außer die Informationen protokollieren)), sodass der Destruktor perfekt funktioniert. In den Situationen, in denen Sie sich dafür interessieren, haben Sie die Möglichkeit, close () manuell aufzurufen und zu überprüfen, ob das Schließen funktioniert (und wenn Sie die 2AM-SMS nicht senden, um sie darüber zu informieren, dass das Dateisystem heruntergefahren ist und sofort ersetzt werden muss). Martin York vor 10 Jahren 0
Re. Ausnahmen: Der nächste (il) logische Schritt ist das Einfügen eines try / catch-Konstrukts, sodass die überflüssige 'close ()' - Funktion immer noch aufgerufen wird, wenn eine Ausnahme ausgelöst wird. Roddy vor 10 Jahren 0
Ein Grund, den der Projektmanager gab, war, dass wir unter schwierigen Bedingungen, in denen Dateihandles knapp sind, Dateien schließen müssen, sobald wir damit fertig sind. Indem Sie eine Regel erstellen, mit der Sie jeden Stream schließen, unabhängig davon, wie trivial die Funktion ist, zwingen Sie den Entwickler, über die Lebensdauer des Objekts nachzudenken. dreamlax vor 10 Jahren 0
@dreamlax Aus diesem Grund sollten sich Entwicklungsorganisationen vor dem Start der Verwendung von C ++ als legitim erweisen. Es ist keine Sprache, die ein durchschnittliches Team korrekt verwenden kann. Ich hoffe, Sie konnten sich behaupten und überzeugen, dass RAII in c ++ ein reales Konzept ist. Ich denke, dass die Überprüfung des schlechten Codes davon ausgeht. Andrew T Finnell vor 9 Jahren 0
16
Karl Bielefeldt

Hier gibt es einen Mittelweg. Der Grund, warum die Rezensenten dieses explizite close()"als eine Frage des Stils und der Ausführlichkeit" wollen, ist, dass sie ohne das Lesen des Codes nicht einfach sagen können, ob Sie es auf diese Weise tun wollten oder ob Sie es völlig vergessen und nur Glück hatten . Es ist auch möglich, dass ihre Egos verletzt wurden, weil sie es nicht bemerkten oder sich daran erinnerten, dass close()der Destruktor angerufen wurde. Das Hinzufügen eines Kommentars, den der Destruktor aufruft, close()ist keine schlechte Idee. Es ist ein wenig unentgeltlich, aber wenn Ihre Kollegen jetzt Aufklärung und / oder Beruhigung benötigen, ist die Wahrscheinlichkeit groß, dass ein zufälliger Betreuer in ein paar Jahren auch noch kommt, vor allem, wenn Ihr Team keine großen E / A-Vorgänge durchführt.

In C ++ sollte es keine "nur Glück" -Ressourcen geben. Es sollte "nur funktionieren". So ist RAII konzipiert und funktioniert bei richtiger Anwendung. Sebastian Redl vor 7 Jahren 0
Ich stimme dir nicht zu. Dies ist C ++. Wenn Sie einen Kommentar benötigen, der Sie daran erinnert, dass ein Fstream von alleine geschlossen wird, sind Sie kein C ++ - Programmierer. jliv902 vor 6 Jahren 5
9
ChrisW

I agree with Loki's answer: the difference between calling close explicitly, and letting the destructor call close, is that the destructor will implicitly catch (i.e. conceal) any exception thrown by close.

The destructor must do this (not propagate exceptions) because it may be called if/while there is an exception already being thrown; and throwing a 2nd exception during a 1st exception is fatal (therefore, all destructors should avoid throwing exceptions).

Unlike Loki I would argue that you do want to call close explicitly, precisely because you do want any exception from close to be visible. For example, perhaps the data is important and you want it written to disk; perhaps the disk is full, the << output operator is written to in-memory cache, and no-one notices that the disk is full until close implicitly calls flush. You're not allowed to return false because false is defined as meaning that the file couldn't be opened. IMO the only sane/safe thing you can do, then, is throw an exception.

It's up to the caller to catch any exception; having no exception thrown should be a guarantee that close was successful and the data safely written to the O/S.

8
grokus

Ich bin an diesem gerissen. Du bist absolut richtig. Wenn jedoch ein Codierungsstandard das explizite Aufrufen von close () erfordert oder wenn sich die Leute darüber einig sind, können Sie nicht viel tun. Wenn ich Sie wäre, würde ich einfach mitgehen. Solche Argumente zu argumentieren ist unproduktiv.

Das Problem mit dem Konsens von kleinen Gruppen (dh Entwicklungsteams). Es kann oft falsch sein, weil die Gruppe von der lautesten Stimme beeinflusst wird. Damit der Konsens der Gruppe funktioniert, muss die Gruppe groß genug sein, um lauten Personen durch die richtige Entscheidung vieler Menschen entgegenzuwirken (dies ist der Grund, warum SO arbeitet (laute Menschen erhalten eine erste Abstimmung, aber in der Regel die richtige Antwort.) die Stimmen (eventuell))). Martin York vor 10 Jahren 9
@Martin: Schöne verschachtelte Klammern. GManNickG vor 10 Jahren 1
@grokus Ich stimme mit Tux-Ds Gefühl überein. Aber ich glaube nicht an Design / Review durch das Komitee. Wenn jemand die Möglichkeit hat, diese Art von Standards zu diktieren, muss er auf andere Weise gezeigt oder als falsch befunden werden. Die Strategie dafür hängt vollständig vom Individuum ab. Das einzige, was in großen Meetings passieren wird, ist zu streiten, und schließlich wird das Volumen zunehmen, bis ein Mediator eintritt und die Entscheidung für Sie trifft. Die beste Antwort darauf ist, eine App zu erstellen, in der .close () aufgerufen wird und wirft. Zeigen Sie, dass es die App beendet. Dann geben Sie ihnen die Wahl, behandeln alle Fehler oder verwenden Sie RAII. Andrew T Finnell vor 9 Jahren 0
4
Patrick Fromberg

Ich glaube, Sie stellen zwei Fragen in einer. Sollten Sie Ausnahmen oder Rückgabewerte verwenden? Sollten Sie RAII verwenden oder nicht?

Wenn in Ihrem Unternehmen keine Ausnahmen zulässig sind, fstream::exceptions()müssen Sie für Ihr Projekt global festgelegt werden. Und Sie müssen auch das Fehlerflag abfragen.

Wenn RAII in Ihrem Unternehmen nicht zulässig istdann nicht C ++ verwenden. Wenn Sie RAII verwenden, wird jede im Destruktor geworfene Ausnahme verschluckt. Das hört sich schrecklich an und ist es auch. Ich stimme jedoch mit anderen darin überein, dass RAII der richtige Weg ist, da jede Fehlerbehandlung hier nicht sinnvoll ist und nicht lesbaren Code erzeugt. Dies liegt daran, dass "Flush" nicht das tut, was Sie vielleicht denken. Sie weist das Betriebssystem an, dies in Ihrem Namen zu tun. Das Betriebssystem wird es tun, wenn es der Meinung ist, dass es praktisch ist. Wenn die Operation leert (dies kann eine Minute dauern, nachdem Ihre Funktion zurückgekehrt ist), können auf Hardwareebene ähnliche Vorgänge auftreten. Die Festplatte verfügt möglicherweise über einen SSD-Cache, der später auf die rotierenden Festplatten geleert wird (was nachts passieren kann, wenn sie weniger ausgelastet ist). Zuletzt enden die Daten auf der Platte. Aber die Geschichte endet nicht hier. Die Daten wurden möglicherweise korrekt gespeichert, aber die Festplatte wird durch eine der vielen möglichen Ursachen zerstört. Wenn RAII für Sie nicht transaktionssicher genug ist, müssen Sie auf jeden Fall zu einem niedrigeren API-Level wechseln, und selbst das wird nicht perfekt sein. Tut mir leid, dass ich hier ein Partykämpfer bin.

1
Toby Speight

Sie müssen auch überprüfen, ob die Schreiboperationen ( <<) erfolgreich waren. Statt zu prüfen is_open(), durchlaufen Sie einfach die ganze Reihe von Operationen und prüfen Sie failbitam Ende:

bool save_data() const
{
    std::ofstream saveFile(m_filename);

    saveFile << m_member1 << '\n'
             << m_member2 << '\n';

    saveFile.close(); // may set failbit

    return saveFile;
}

Wenn es nicht dringend erforderlich ist, jede Zeile zu leeren, wie sie geschrieben wurde, sie lieber zu verwenden '\n'als zu verwenden std::endl(wie ich es gezeigt habe), und close()alles auf einmal schreiben zu lassen, kann dies die Geschwindigkeit erheblich beeinträchtigen, insbesondere wenn dies der Fall ist eine große Anzahl von Zeilen, die geschrieben werden sollen.

0
Francis Cugler

Nachdem ich die Frage und die Antworten gelesen hatte, kam ich zu dem Schluss, dass hier Kommentare zum Tragen kommen können. Ich hatte vor kurzem diese Q / A- Ratschläge gelesen , aber einige Kommentare und die akzeptierte Antwort gaben mir einen Einblick in die Situation. Verwenden Sie Kommentare, um das Warum zu erklären. Lassen Sie den Code erklären, wie. Ich kann Ihre Funktion für jeden Fall ähnlich als Beispiel verwenden:

bool SomeClass::saveData() {
    std::ofstream saveFile(m_filename);

    if (!saveFile.is_open())
        return false;

    saveFile << m_member1 << std::endl; // I would replace `endl` with `'\n'`
    saveFile << m_member2 << std::endl; // for performance reasons. 

    // I intentionally want to close the file before the closing scope to
    // free up limited resources and to log potential exceptions and errors.  
    saveFile.close(); 
    return true;
} 

bool SomeClass::saveData() {
    std::ofstream saveFile(m_filename);

    if (!saveFile.is_open())
        return false;

    saveFile << m_member1 << std::endl; // I would replace `endl` with `'\n'`
    saveFile << m_member2 << std::endl; // for performance reasons. 

    // Resources and errors are not a concern: relying on RAII no need
    // to call file.close();
    return true;
}

Dann könnte Ihnen die richtige Art des Kommentierens aus triftigen Gründen hilfreich sein. Dann zumindest auf diese Weise; man würde wissen, dass Sie vorhaben, es anzurufen oder wegzulassen, und warum! Gut geschriebener Code ist das, wie und sollte keine Kommentare benötigen.