From 680ec28b577a45eecf800e39bb2dff4dde1bb270 Mon Sep 17 00:00:00 2001 From: erius Date: Sat, 26 Oct 2024 04:23:11 +0300 Subject: [PATCH] Added file_name and temp fields to FileTape Removed explicit keyword before every constructor in FileTape Added getters for private fields in FileTape Made Tape's copy construcot marked as delete by default Implemented custom destrucor for FileTape Added explicit move and copy constructors to FileTape Added generate_tmp_file_name method to tape namespace Removed assets directory from tests - testing data should be provided in the test's cpp file --- include/tapelib/filetape.h | 35 ++++++++++++++++++++----- include/tapelib/tape.h | 13 +++++++--- include/tapelib/tape_util.h | 2 +- src/filetape.cpp | 40 +++++++++++++++++++---------- tests/assets/filetape_test_data.txt | 5 ---- tests/filetape_tests.cpp | 12 +++++++++ 6 files changed, 78 insertions(+), 29 deletions(-) delete mode 100644 tests/assets/filetape_test_data.txt diff --git a/include/tapelib/filetape.h b/include/tapelib/filetape.h index 27e1a9f..b8ba77e 100644 --- a/include/tapelib/filetape.h +++ b/include/tapelib/filetape.h @@ -52,10 +52,22 @@ const static int FT_SEEK_OFFSET = FT_CELL_SIZE + 1; */ class FileTape : public Tape { private: + std::string file_name; std::fstream file; FileTapeSettings settings; + bool temp = false; public: + /** + * Creating copies of FileTape creates 2+ fstreams to one file, which + * could introduce IO errors or data races if implemented incorrectly - + * mark the copy constructor as deleted. Moving is fine. + */ + FileTape(const FileTape &) = delete; + FileTape(FileTape &&) noexcept = default; + FileTape &operator=(const FileTape &) = delete; + FileTape &operator=(FileTape &&tape) = default; + /** * Initializes a new instance of FileTape that will open an existing file * with the specified name, already filled with data. File must be read and @@ -64,8 +76,8 @@ class FileTape : public Tape { * @param file_name Name of a file to be opened. * @param settings FileTape settings */ - explicit FileTape(const std::string &file_name, - FileTapeSettings settings = FT_DEFAULT_SETTINGS); + FileTape(std::string file_name, + FileTapeSettings settings = FT_DEFAULT_SETTINGS); /** * Initializes a new instance of FileTape that will create and open a file @@ -76,8 +88,8 @@ class FileTape : public Tape { * @param file_name Name of a file to be created and opened * @param settings FileTape settings */ - explicit FileTape(size_t size, const std::string &file_name, - FileTapeSettings settings = FT_DEFAULT_SETTINGS); + FileTape(size_t size, std::string file_name, + FileTapeSettings settings = FT_DEFAULT_SETTINGS); /** * Initializes a new instance of FileTape that will create and open a @@ -86,8 +98,7 @@ class FileTape : public Tape { * @param size Size of a newly created FileTape * @param settings FileTape settings */ - explicit FileTape(size_t size, - FileTapeSettings settings = FT_DEFAULT_SETTINGS); + FileTape(size_t size, FileTapeSettings settings = FT_DEFAULT_SETTINGS); /** * Advances the underlying fstream to a new line. @@ -116,6 +127,18 @@ class FileTape : public Tape { * @param data Number, that should be written to the current line. */ void write(uint32_t data) override; + + ~FileTape() override; + + // Define getters in header file since they are implicitly inlined + + bool is_temp() const { return this->temp; } + + const FileTapeSettings &get_settings() const { return this->settings; } + + const std::string &get_file_name() const { return this->file_name; } + + const std::fstream &get_file() const { return this->file; } }; } // namespace tape diff --git a/include/tapelib/tape.h b/include/tapelib/tape.h index 5c7667a..b774043 100644 --- a/include/tapelib/tape.h +++ b/include/tapelib/tape.h @@ -17,11 +17,16 @@ namespace tape { */ class Tape { public: + /** + * Compiler provided constructors and destructor definitions. + * Make move constructor and destructor default, copy construcotr delete, + * and allow destrucor to be overriden. + */ Tape() = default; - Tape(const Tape &) = default; - Tape(Tape &&) = delete; - Tape &operator=(const Tape &) = default; - Tape &operator=(Tape &&) = delete; + Tape(const Tape &) = delete; + Tape(Tape &&) = default; + Tape &operator=(const Tape &) = delete; + Tape &operator=(Tape &&) = default; virtual ~Tape() = default; /** diff --git a/include/tapelib/tape_util.h b/include/tapelib/tape_util.h index 1433961..2b6e74e 100644 --- a/include/tapelib/tape_util.h +++ b/include/tapelib/tape_util.h @@ -7,6 +7,6 @@ namespace tape { void sort(Tape &input, Tape &output); -} +} // namespace tape #endif diff --git a/src/filetape.cpp b/src/filetape.cpp index 8138e95..f50222d 100644 --- a/src/filetape.cpp +++ b/src/filetape.cpp @@ -1,12 +1,17 @@ #include "filetape.h" +#include #include using std::fstream; using tape::FileTape; -FileTape::FileTape(const std::string &file_name, FileTapeSettings settings) - : settings(settings) { - this->file = fstream(file_name); +std::string generate_tmp_file_name() { + std::array tmp_file_name{}; + // tmpnam is unsafe, but its cross-platform + // TODO: make a safe, cross-platform tmp file handler + // (possibly utilizing std::filesystem::temp_directory_path()) + std::tmpnam(tmp_file_name.data()); + return tmp_file_name.data(); } void fill_file_with_empty_cells(size_t size, fstream &file) { @@ -18,23 +23,25 @@ void fill_file_with_empty_cells(size_t size, fstream &file) { file.seekg(0, fstream::beg); } -FileTape::FileTape(size_t size, const std::string &file_name, +FileTape::FileTape(std::string file_name, FileTapeSettings settings) + : settings(settings), file_name(file_name) { + this->file = fstream(file_name); +} + +FileTape::FileTape(size_t size, std::string file_name, FileTapeSettings settings) : FileTape(file_name, settings) { this->file = - fstream(file_name, fstream::in | fstream::out | fstream::trunc); - fill_file_with_empty_cells(size, file); + fstream(this->file_name, fstream::in | fstream::out | fstream::trunc); + fill_file_with_empty_cells(size, this->file); } FileTape::FileTape(size_t size, FileTapeSettings settings) - : settings(settings) { - std::array file_name{}; - // tmpnam is unsafe, but its cross-platform - // create a platform dependant tmp file handler later - std::tmpnam(file_name.data()); + : settings(settings), temp(true) { + this->file_name = generate_tmp_file_name(); this->file = - fstream(file_name.data(), fstream::in | fstream::out | fstream::trunc); - fill_file_with_empty_cells(size, file); + fstream(this->file_name, fstream::in | fstream::out | fstream::trunc); + fill_file_with_empty_cells(size, this->file); } bool FileTape::seek_backwards() { @@ -79,3 +86,10 @@ void FileTape::write(uint32_t data) { this->file.write(cell.str().data(), FT_CELL_SIZE); this->file.seekg(-FT_SEEK_OFFSET + 1, fstream::cur); } + +FileTape::~FileTape() { + this->file.close(); + if (this->is_temp()) { + std::filesystem::remove(this->file_name); + } +} diff --git a/tests/assets/filetape_test_data.txt b/tests/assets/filetape_test_data.txt deleted file mode 100644 index 176a180..0000000 --- a/tests/assets/filetape_test_data.txt +++ /dev/null @@ -1,5 +0,0 @@ -0000000001 -0000000002 -0000000003 -0000000004 -0000000005 diff --git a/tests/filetape_tests.cpp b/tests/filetape_tests.cpp index 3f7cf25..4c12ba2 100644 --- a/tests/filetape_tests.cpp +++ b/tests/filetape_tests.cpp @@ -1,6 +1,18 @@ #include "filetape.h" #include +const static std::string TEST_DATA = "0000000001\n" + "0000012345\n" + "0000000000\n" + "2222222222\n" + "4294967295"; + +tape::FileTape prepare_test_filetape() { + tape::FileTape tape(0); + + return tape; +} + // NOLINTBEGIN(readability-function-cognitive-complexity) TEST_CASE("Reading data from a FileTape", "[filetape]") { tape::FileTape tape(FILETAPE_TEST_FILE);