From b00476abac76dc44f024f5da8fe64c05d76c86f0 Mon Sep 17 00:00:00 2001 From: Lucas CHOLLET Date: Sun, 5 Nov 2023 15:31:17 -0500 Subject: [PATCH] AK: Use an enum to specify the open mode instead of a bool Let's replace this bool with an `enum class` in order to enhance readability. This is done by repurposing `MappedFile`'s `OpenMode` into a shared `enum` simply called `Mode`. --- AK/MemoryStream.cpp | 4 ++-- AK/MemoryStream.h | 7 ++++++- Tests/AK/TestMemoryStream.cpp | 2 +- Tests/LibCore/TestLibCoreMappedFile.cpp | 8 ++++---- Userland/Libraries/LibAudio/Loader.cpp | 2 +- Userland/Libraries/LibCore/MappedFile.cpp | 14 +++++++------- Userland/Libraries/LibCore/MappedFile.h | 12 +++--------- 7 files changed, 24 insertions(+), 25 deletions(-) diff --git a/AK/MemoryStream.cpp b/AK/MemoryStream.cpp index 530395a66bf..a4baeb819e6 100644 --- a/AK/MemoryStream.cpp +++ b/AK/MemoryStream.cpp @@ -12,9 +12,9 @@ namespace AK { -FixedMemoryStream::FixedMemoryStream(Bytes bytes, bool writing_enabled) +FixedMemoryStream::FixedMemoryStream(Bytes bytes, Mode mode) : m_bytes(bytes) - , m_writing_enabled(writing_enabled) + , m_writing_enabled(mode == Mode::ReadWrite) { } diff --git a/AK/MemoryStream.h b/AK/MemoryStream.h index 383d03336dd..192de0febbe 100644 --- a/AK/MemoryStream.h +++ b/AK/MemoryStream.h @@ -18,7 +18,12 @@ namespace AK { /// using a single read/write head. class FixedMemoryStream : public SeekableStream { public: - explicit FixedMemoryStream(Bytes bytes, bool writing_enabled = true); + enum class Mode { + ReadOnly, + ReadWrite, + }; + + explicit FixedMemoryStream(Bytes bytes, Mode mode = Mode::ReadWrite); explicit FixedMemoryStream(ReadonlyBytes bytes); virtual bool is_eof() const override; diff --git a/Tests/AK/TestMemoryStream.cpp b/Tests/AK/TestMemoryStream.cpp index 3a01076bd70..306cab6908e 100644 --- a/Tests/AK/TestMemoryStream.cpp +++ b/Tests/AK/TestMemoryStream.cpp @@ -259,7 +259,7 @@ TEST_CASE(fixed_memory_read_in_place) EXPECT_EQ(characters, some_words.bytes()); EXPECT(readonly_stream.is_eof()); - FixedMemoryStream mutable_stream { Bytes { const_cast(some_words.bytes().data()), some_words.bytes().size() }, true }; + FixedMemoryStream mutable_stream { Bytes { const_cast(some_words.bytes().data()), some_words.bytes().size() }, FixedMemoryStream::Mode::ReadWrite }; // Trying to read mutable values from a mutable stream should succeed. TRY_OR_FAIL(mutable_stream.read_in_place(1)); EXPECT_EQ(mutable_stream.offset(), 1u); diff --git a/Tests/LibCore/TestLibCoreMappedFile.cpp b/Tests/LibCore/TestLibCoreMappedFile.cpp index 8f2aeb33c5e..bfe0909f5cf 100644 --- a/Tests/LibCore/TestLibCoreMappedFile.cpp +++ b/Tests/LibCore/TestLibCoreMappedFile.cpp @@ -27,7 +27,7 @@ TEST_CASE(mapped_file_open) TRY_OR_FAIL(maybe_file.value()->write_until_depleted(text.bytes())); } - auto maybe_file = Core::MappedFile::map("/tmp/file-open-test.txt"sv, Core::MappedFile::OpenMode::ReadWrite); + auto maybe_file = Core::MappedFile::map("/tmp/file-open-test.txt"sv, Core::MappedFile::Mode::ReadWrite); if (maybe_file.is_error()) { warnln("Failed to open the file: {}", strerror(maybe_file.error().code())); VERIFY_NOT_REACHED(); @@ -46,7 +46,7 @@ constexpr auto expected_buffer_contents = "<small>(Please consider transla TEST_CASE(mapped_file_read_bytes) { - auto file = TRY_OR_FAIL(Core::MappedFile::map("/usr/Tests/LibCore/long_lines.txt"sv, Core::MappedFile::OpenMode::ReadOnly)); + auto file = TRY_OR_FAIL(Core::MappedFile::map("/usr/Tests/LibCore/long_lines.txt"sv, Core::MappedFile::Mode::ReadOnly)); auto buffer = TRY_OR_FAIL(ByteBuffer::create_uninitialized(131)); @@ -63,7 +63,7 @@ constexpr auto expected_seek_contents3 = "levels of advanc"sv; TEST_CASE(mapped_file_seeking_around) { - auto file = TRY_OR_FAIL(Core::MappedFile::map("/usr/Tests/LibCore/long_lines.txt"sv, Core::MappedFile::OpenMode::ReadOnly)); + auto file = TRY_OR_FAIL(Core::MappedFile::map("/usr/Tests/LibCore/long_lines.txt"sv, Core::MappedFile::Mode::ReadOnly)); EXPECT_EQ(file->size().release_value(), 8702ul); @@ -89,7 +89,7 @@ TEST_CASE(mapped_file_seeking_around) BENCHMARK_CASE(file_tell) { - auto file = TRY_OR_FAIL(Core::MappedFile::map("/usr/Tests/LibCore/10kb.txt"sv, Core::MappedFile::OpenMode::ReadOnly)); + auto file = TRY_OR_FAIL(Core::MappedFile::map("/usr/Tests/LibCore/10kb.txt"sv, Core::MappedFile::Mode::ReadOnly)); auto expected_file_offset = 0u; auto ten_byte_buffer = TRY_OR_FAIL(ByteBuffer::create_uninitialized(1)); for (auto i = 0u; i < 4000; ++i) { diff --git a/Userland/Libraries/LibAudio/Loader.cpp b/Userland/Libraries/LibAudio/Loader.cpp index 31dbd2e84cf..c504b028bd7 100644 --- a/Userland/Libraries/LibAudio/Loader.cpp +++ b/Userland/Libraries/LibAudio/Loader.cpp @@ -44,7 +44,7 @@ static constexpr LoaderPluginInitializer s_initializers[] = { ErrorOr, LoaderError> Loader::create(StringView path) { - auto stream = TRY(Core::MappedFile::map(path, Core::MappedFile::OpenMode::ReadOnly)); + auto stream = TRY(Core::MappedFile::map(path, Core::MappedFile::Mode::ReadOnly)); return adopt_ref(*new (nothrow) Loader(TRY(Loader::create_plugin(move(stream))))); } diff --git a/Userland/Libraries/LibCore/MappedFile.cpp b/Userland/Libraries/LibCore/MappedFile.cpp index 4d8262ea36c..802472f0b70 100644 --- a/Userland/Libraries/LibCore/MappedFile.cpp +++ b/Userland/Libraries/LibCore/MappedFile.cpp @@ -15,9 +15,9 @@ namespace Core { -ErrorOr> MappedFile::map(StringView path, OpenMode mode) +ErrorOr> MappedFile::map(StringView path, Mode mode) { - auto const file_mode = mode == OpenMode::ReadOnly ? O_RDONLY : O_RDWR; + auto const file_mode = mode == Mode::ReadOnly ? O_RDONLY : O_RDWR; auto fd = TRY(Core::System::open(path, file_mode | O_CLOEXEC, 0)); return map_from_fd_and_close(fd, path, mode); } @@ -27,7 +27,7 @@ ErrorOr> MappedFile::map_from_file(NonnullOwnPtrleak_fd(Badge {}), path); } -ErrorOr> MappedFile::map_from_fd_and_close(int fd, [[maybe_unused]] StringView path, OpenMode mode) +ErrorOr> MappedFile::map_from_fd_and_close(int fd, [[maybe_unused]] StringView path, Mode mode) { TRY(Core::System::fcntl(fd, F_SETFD, FD_CLOEXEC)); @@ -41,11 +41,11 @@ ErrorOr> MappedFile::map_from_fd_and_close(int fd, [[m int protection; int flags; switch (mode) { - case OpenMode::ReadOnly: + case Mode::ReadOnly: protection = PROT_READ; flags = MAP_SHARED; break; - case OpenMode::ReadWrite: + case Mode::ReadWrite: protection = PROT_READ | PROT_WRITE; // Don't map a read-write mapping shared as a precaution. flags = MAP_PRIVATE; @@ -57,8 +57,8 @@ ErrorOr> MappedFile::map_from_fd_and_close(int fd, [[m return adopt_own(*new MappedFile(ptr, size, mode)); } -MappedFile::MappedFile(void* ptr, size_t size, OpenMode mode) - : FixedMemoryStream(Bytes { ptr, size }, mode == OpenMode::ReadWrite) +MappedFile::MappedFile(void* ptr, size_t size, Mode mode) + : FixedMemoryStream(Bytes { ptr, size }, mode) , m_data(ptr) , m_size(size) { diff --git a/Userland/Libraries/LibCore/MappedFile.h b/Userland/Libraries/LibCore/MappedFile.h index 1863b14e41f..355484ce074 100644 --- a/Userland/Libraries/LibCore/MappedFile.h +++ b/Userland/Libraries/LibCore/MappedFile.h @@ -22,15 +22,9 @@ class MappedFile : public FixedMemoryStream { AK_MAKE_NONMOVABLE(MappedFile); public: - // Reflects a simplified version of mmap protection and flags. - enum class OpenMode { - ReadOnly, - ReadWrite, - }; - - static ErrorOr> map(StringView path, OpenMode mode = OpenMode::ReadOnly); + static ErrorOr> map(StringView path, Mode mode = Mode::ReadOnly); static ErrorOr> map_from_file(NonnullOwnPtr, StringView path); - static ErrorOr> map_from_fd_and_close(int fd, StringView path, OpenMode mode = OpenMode::ReadOnly); + static ErrorOr> map_from_fd_and_close(int fd, StringView path, Mode mode = Mode::ReadOnly); virtual ~MappedFile(); // Non-stream APIs for using MappedFile as a simple POSIX API wrapper. @@ -39,7 +33,7 @@ public: ReadonlyBytes bytes() const { return { m_data, m_size }; } private: - explicit MappedFile(void*, size_t, OpenMode); + explicit MappedFile(void*, size_t, Mode); void* m_data { nullptr }; size_t m_size { 0 };