From 9abc9a59b945362f84db2d43b05bb4eda1862c2b Mon Sep 17 00:00:00 2001 From: jfcarroll Date: Thu, 3 Feb 2022 09:57:21 -0800 Subject: [PATCH] Combine InputFile and OutputFile into a single class. PiperOrigin-RevId: 426172033 --- connections/BUILD | 1 + connections/implementation/BUILD | 1 + .../platform/implementation/g3/platform.cc | 6 +- .../ios/Source/Internal/platform.mm | 4 +- internal/platform/implementation/shared/BUILD | 1 + .../platform/implementation/shared/file.cc | 41 +++++---- .../platform/implementation/shared/file.h | 33 +++---- .../implementation/shared/file_test.cc | 87 ++++++++++--------- .../windows/bluetooth_classic_medium.cc | 4 +- .../windows/condition_variable_test.cc | 2 +- .../platform/implementation/windows/mutex.h | 3 + .../implementation/windows/mutex_test.cc | 25 +++--- .../implementation/windows/platform.cc | 6 +- .../implementation/windows/test_utils.cc | 10 ++- 14 files changed, 115 insertions(+), 109 deletions(-) diff --git a/connections/BUILD b/connections/BUILD index 2ad89516..e3e5f3be 100644 --- a/connections/BUILD +++ b/connections/BUILD @@ -69,6 +69,7 @@ cc_library( "//internal/platform/implementation/ios:__subpackages__", ], deps = [ + "//base", "//internal/platform:base", "//internal/platform:types", "//internal/platform:util", diff --git a/connections/implementation/BUILD b/connections/implementation/BUILD index 3f40d31f..6ce1241a 100644 --- a/connections/implementation/BUILD +++ b/connections/implementation/BUILD @@ -90,6 +90,7 @@ cc_library( ], deps = [ ":message_lite", + "//base", "//connections:core_types", "//connections/implementation/mediums", "//connections/implementation/mediums:utils", diff --git a/internal/platform/implementation/g3/platform.cc b/internal/platform/implementation/g3/platform.cc index 7f1ff88d..49842156 100644 --- a/internal/platform/implementation/g3/platform.cc +++ b/internal/platform/implementation/g3/platform.cc @@ -103,13 +103,13 @@ std::unique_ptr ImplementationPlatform::CreateAtomicBoolean( std::unique_ptr ImplementationPlatform::CreateInputFile( PayloadId payload_id, std::int64_t total_size) { - return absl::make_unique(GetPayloadPath(payload_id), - total_size); + return shared::IOFile::CreateInputFile(GetPayloadPath(payload_id), + total_size); } std::unique_ptr ImplementationPlatform::CreateOutputFile( PayloadId payload_id) { - return absl::make_unique(GetPayloadPath(payload_id)); + return shared::IOFile::CreateOutputFile(GetPayloadPath(payload_id)); } std::unique_ptr ImplementationPlatform::CreateLogMessage( diff --git a/internal/platform/implementation/ios/Source/Internal/platform.mm b/internal/platform/implementation/ios/Source/Internal/platform.mm index e7ef2b81..a7366bb9 100644 --- a/internal/platform/implementation/ios/Source/Internal/platform.mm +++ b/internal/platform/implementation/ios/Source/Internal/platform.mm @@ -88,12 +88,12 @@ std::unique_ptr ImplementationPlatform::CreateInputFile(PayloadId pay if (url != nil) { return absl::make_unique(url); } else { - return absl::make_unique(GetPayloadPath(payload_id), total_size); + return shared::IOFile::CreateInputFile(GetPayloadPath(payload_id), total_size); } } std::unique_ptr ImplementationPlatform::CreateOutputFile(PayloadId payload_id) { - return absl::make_unique(GetPayloadPath(payload_id)); + return shared::IOFile::CreateOutputFile(GetPayloadPath(payload_id)); } std::unique_ptr ImplementationPlatform::CreateLogMessage( diff --git a/internal/platform/implementation/shared/BUILD b/internal/platform/implementation/shared/BUILD index 3d7417e9..8c9aa3de 100644 --- a/internal/platform/implementation/shared/BUILD +++ b/internal/platform/implementation/shared/BUILD @@ -51,6 +51,7 @@ cc_library( deps = [ "//internal/platform:base", "//internal/platform/implementation:types", + "@com_google_absl//absl/memory", "@com_google_absl//absl/strings", ], ) diff --git a/internal/platform/implementation/shared/file.cc b/internal/platform/implementation/shared/file.cc index 9b1c3a70..12a5e8d1 100644 --- a/internal/platform/implementation/shared/file.cc +++ b/internal/platform/implementation/shared/file.cc @@ -17,6 +17,7 @@ #include #include +#include "absl/memory/memory.h" #include "absl/strings/string_view.h" #include "internal/platform/exception.h" @@ -25,11 +26,27 @@ namespace nearby { namespace shared { // InputFile +std::unique_ptr IOFile::CreateInputFile(const absl::string_view path, + size_t size) { + return absl::WrapUnique(new IOFile(path, size)); +} -InputFile::InputFile(const std::string& path, std::int64_t size) - : file_(path, std::ios::binary), path_(path), total_size_(size) {} +IOFile::IOFile(const absl::string_view path, size_t size) + : file_(std::string(path.data(), path.size()), + std::ios::binary | std::ios::in), + path_(path), + total_size_(size) {} -ExceptionOr InputFile::Read(std::int64_t size) { +std::unique_ptr IOFile::CreateOutputFile(const absl::string_view path) { + return std::unique_ptr(new IOFile(path)); +} + +IOFile::IOFile(const absl::string_view path) + : file_(std::string(path.data(), path.size()), + std::ios::binary | std::ios::out | std::ios::trunc), + total_size_(0) {} + +ExceptionOr IOFile::Read(std::int64_t size) { if (!file_.is_open()) { return ExceptionOr{Exception::kIo}; } @@ -53,19 +70,14 @@ ExceptionOr InputFile::Read(std::int64_t size) { return ExceptionOr(ByteArray(read_bytes.get(), num_bytes_read)); } -Exception InputFile::Close() { +Exception IOFile::Close() { if (file_.is_open()) { file_.close(); } return {Exception::kSuccess}; } -// OutputFile - -OutputFile::OutputFile(absl::string_view path) - : file_(std::string(path), std::ios::binary) {} - -Exception OutputFile::Write(const ByteArray& data) { +Exception IOFile::Write(const ByteArray& data) { if (!file_.is_open()) { return {Exception::kIo}; } @@ -79,18 +91,11 @@ Exception OutputFile::Write(const ByteArray& data) { return {file_.good() ? Exception::kSuccess : Exception::kIo}; } -Exception OutputFile::Flush() { +Exception IOFile::Flush() { file_.flush(); return {file_.good() ? Exception::kSuccess : Exception::kIo}; } -Exception OutputFile::Close() { - if (file_.is_open()) { - file_.close(); - } - return {Exception::kSuccess}; -} - } // namespace shared } // namespace nearby } // namespace location diff --git a/internal/platform/implementation/shared/file.h b/internal/platform/implementation/shared/file.h index e215902e..cfd912f5 100644 --- a/internal/platform/implementation/shared/file.h +++ b/internal/platform/implementation/shared/file.h @@ -27,37 +27,28 @@ namespace location { namespace nearby { namespace shared { -class InputFile final : public api::InputFile { +class IOFile final : public api::InputFile, public api::OutputFile { public: - explicit InputFile(const std::string& path, std::int64_t size); - ~InputFile() override = default; - InputFile(InputFile&&) = default; - InputFile& operator=(InputFile&&) = default; + static std::unique_ptr CreateInputFile(const absl::string_view path, + size_t size); + static std::unique_ptr CreateOutputFile(const absl::string_view path); ExceptionOr Read(std::int64_t size) override; - std::string GetFilePath() const override { return path_; } + std::string GetFilePath() const override { + return std::string(path_.data(), path_.size()); + } std::int64_t GetTotalSize() const override { return total_size_; } Exception Close() override; - private: - std::ifstream file_; - std::string path_; - std::int64_t total_size_; -}; - -class OutputFile final : public api::OutputFile { - public: - explicit OutputFile(absl::string_view path); - ~OutputFile() override = default; - OutputFile(OutputFile&&) = default; - OutputFile& operator=(OutputFile&&) = default; - Exception Write(const ByteArray& data) override; Exception Flush() override; - Exception Close() override; private: - std::ofstream file_; + explicit IOFile(const absl::string_view path, size_t size); + explicit IOFile(const absl::string_view path); + std::fstream file_; + absl::string_view path_; + std::int64_t total_size_; }; } // namespace shared diff --git a/internal/platform/implementation/shared/file_test.cc b/internal/platform/implementation/shared/file_test.cc index 01b95054..367e9ff0 100644 --- a/internal/platform/implementation/shared/file_test.cc +++ b/internal/platform/implementation/shared/file_test.cc @@ -64,83 +64,84 @@ class FileTest : public ::testing::Test { size_t size_ = 0; }; -TEST_F(FileTest, InputFile_NonExistentPath) { - InputFile input_file("/not/a/valid/path.txt", GetSize()); - ExceptionOr read_result = input_file.Read(kMaxSize); +TEST_F(FileTest, IOFile_NonExistentPathInput) { + auto io_file = + shared::IOFile::CreateInputFile("/not/a/valid/path.txt", GetSize()); + ExceptionOr read_result = io_file->Read(kMaxSize); EXPECT_FALSE(read_result.ok()); EXPECT_TRUE(read_result.GetException().Raised(Exception::kIo)); } -TEST_F(FileTest, InputFile_GetFilePath) { - InputFile input_file(path_, GetSize()); - EXPECT_EQ(input_file.GetFilePath(), path_); +TEST_F(FileTest, IOFile_GetFilePath) { + auto io_file = shared::IOFile::CreateInputFile(path_, GetSize()); + EXPECT_EQ(io_file->GetFilePath(), path_); } -TEST_F(FileTest, InputFile_EmptyFileEOF) { - InputFile input_file(path_, GetSize()); - AssertEmpty(input_file.Read(kMaxSize)); +TEST_F(FileTest, IOFile_EmptyFileEOF) { + auto io_file = shared::IOFile::CreateInputFile(path_, GetSize()); + AssertEmpty(io_file->Read(kMaxSize)); } -TEST_F(FileTest, InputFile_ReadWorks) { +TEST_F(FileTest, IOFile_ReadWorks) { WriteToFile("abc"); - InputFile input_file(path_, GetSize()); - input_file.Read(kMaxSize); + auto io_file = shared::IOFile::CreateInputFile(path_, GetSize()); + io_file->Read(kMaxSize); SUCCEED(); } -TEST_F(FileTest, InputFile_ReadUntilEOF) { +TEST_F(FileTest, IOFile_ReadUntilEOF) { WriteToFile("abc"); - InputFile input_file(path_, GetSize()); - AssertEquals(input_file.Read(kMaxSize), "abc"); - AssertEmpty(input_file.Read(kMaxSize)); + auto io_file = shared::IOFile::CreateInputFile(path_, GetSize()); + AssertEquals(io_file->Read(kMaxSize), "abc"); + AssertEmpty(io_file->Read(kMaxSize)); } -TEST_F(FileTest, InputFile_ReadWithSize) { +TEST_F(FileTest, IOFile_ReadWithSize) { WriteToFile("abc"); - InputFile input_file(path_, GetSize()); - AssertEquals(input_file.Read(2), "ab"); - AssertEquals(input_file.Read(1), "c"); - AssertEmpty(input_file.Read(kMaxSize)); + auto io_file = shared::IOFile::CreateInputFile(path_, GetSize()); + AssertEquals(io_file->Read(2), "ab"); + AssertEquals(io_file->Read(1), "c"); + AssertEmpty(io_file->Read(kMaxSize)); } -TEST_F(FileTest, InputFile_GetTotalSize) { +TEST_F(FileTest, IOFile_GetTotalSize) { WriteToFile("abc"); - InputFile input_file(path_, GetSize()); - EXPECT_EQ(input_file.GetTotalSize(), 3); - AssertEquals(input_file.Read(1), "a"); - EXPECT_EQ(input_file.GetTotalSize(), 3); + auto io_file = shared::IOFile::CreateInputFile(path_, GetSize()); + EXPECT_EQ(io_file->GetTotalSize(), 3); + AssertEquals(io_file->Read(1), "a"); + EXPECT_EQ(io_file->GetTotalSize(), 3); } -TEST_F(FileTest, InputFile_Close) { +TEST_F(FileTest, IOFile_CloseInput) { WriteToFile("abc"); - InputFile input_file(path_, GetSize()); - input_file.Close(); - ExceptionOr read_result = input_file.Read(kMaxSize); + auto io_file = shared::IOFile::CreateInputFile(path_, GetSize()); + io_file->Close(); + ExceptionOr read_result = io_file->Read(kMaxSize); EXPECT_FALSE(read_result.ok()); EXPECT_TRUE(read_result.GetException().Raised(Exception::kIo)); } -TEST_F(FileTest, OutputFile_NonExistentPath) { - OutputFile output_file("/not/a/valid/path.txt"); +TEST_F(FileTest, IOFile_NonExistentPathOutput) { + auto io_file = shared::IOFile::CreateOutputFile("/not/a/valid/path.txt"); ByteArray bytes("a", 1); - EXPECT_TRUE(output_file.Write(bytes).Raised(Exception::kIo)); + EXPECT_TRUE(io_file->Write(bytes).Raised(Exception::kIo)); } -TEST_F(FileTest, OutputFile_Write) { - OutputFile output_file(path_); +TEST_F(FileTest, IOFile_Write) { + auto io_file_output = shared::IOFile::CreateOutputFile(path_); ByteArray bytes1("a"); ByteArray bytes2("bc"); - EXPECT_EQ(output_file.Write(bytes1), Exception{Exception::kSuccess}); - EXPECT_EQ(output_file.Write(bytes2), Exception{Exception::kSuccess}); - InputFile input_file(path_, GetSize()); - AssertEquals(input_file.Read(kMaxSize), "abc"); + EXPECT_EQ(io_file_output->Write(bytes1), Exception{Exception::kSuccess}); + EXPECT_EQ(io_file_output->Write(bytes2), Exception{Exception::kSuccess}); + auto io_file_input = shared::IOFile::CreateInputFile(path_, GetSize()); + AssertEquals(io_file_input->Read(kMaxSize), "abc"); } -TEST_F(FileTest, OutputFile_Close) { - OutputFile output_file(path_); - output_file.Close(); +TEST_F(FileTest, IOFile_CloseOutput) { + auto io_file = shared::IOFile::CreateOutputFile(path_); + io_file->Close(); ByteArray bytes("a"); - EXPECT_EQ(output_file.Write(bytes), Exception{Exception::kIo}); + EXPECT_EQ(io_file->Write(bytes), Exception{Exception::kIo}); } } // namespace shared diff --git a/internal/platform/implementation/windows/bluetooth_classic_medium.cc b/internal/platform/implementation/windows/bluetooth_classic_medium.cc index ce9689e5..a90b01f6 100644 --- a/internal/platform/implementation/windows/bluetooth_classic_medium.cc +++ b/internal/platform/implementation/windows/bluetooth_classic_medium.cc @@ -382,8 +382,8 @@ winrt::fire_and_forget BluetoothClassicMedium::DeviceWatcher_Added( winrt::Windows::Foundation::AsyncStatus status) { EnterCriticalSection(&critical_section_); - std::unique_ptr bluetoothDeviceP = - std::make_unique(bluetoothDevice.get()); + auto bluetoothDeviceP = + absl::WrapUnique(new BluetoothDevice(bluetoothDevice.get())); discovered_devices_by_id_[deviceInfo.Id()] = std::move(bluetoothDeviceP); diff --git a/internal/platform/implementation/windows/condition_variable_test.cc b/internal/platform/implementation/windows/condition_variable_test.cc index f4d9c9b5..03626b5d 100644 --- a/internal/platform/implementation/windows/condition_variable_test.cc +++ b/internal/platform/implementation/windows/condition_variable_test.cc @@ -50,7 +50,7 @@ class ConditionVariableTests : public testing::Test { } void PostEvent() { - std::lock_guard guard(mutex_actual_.GetWindowsMutex()); + absl::MutexLock::MutexLock(&mutex_actual_.GetMutex()); condition_variable_actual_.Notify(); } diff --git a/internal/platform/implementation/windows/mutex.h b/internal/platform/implementation/windows/mutex.h index dd7580a2..42410827 100644 --- a/internal/platform/implementation/windows/mutex.h +++ b/internal/platform/implementation/windows/mutex.h @@ -56,6 +56,9 @@ class ABSL_LOCKABLE Mutex : public api::Mutex { } } + absl::Mutex& GetMutex() { return mutex_; } + std::recursive_mutex& GetRecursiveMutex() { return recursive_mutex_; } + private: friend class ConditionVariable; absl::Mutex mutex_; diff --git a/internal/platform/implementation/windows/mutex_test.cc b/internal/platform/implementation/windows/mutex_test.cc index b05a8943..f8223a3e 100644 --- a/internal/platform/implementation/windows/mutex_test.cc +++ b/internal/platform/implementation/windows/mutex_test.cc @@ -14,7 +14,7 @@ #include "internal/platform/implementation/windows/mutex.h" -#include // NOLINT +#include // NOLINT #include "gtest/gtest.h" @@ -24,18 +24,17 @@ class MutexTests : public testing::Test { public: MutexTest(location::nearby::windows::Mutex& mutex) : mutex_(mutex) {} - std::future WaitForLock() { // NOLINT - return std::async( - std::launch::async, - // for this lambda you need C++14 - [this]() mutable { - std::unique_lock lck(mutex_.GetWindowsMutex()); - return true; - }); + std::future WaitForLock() { // NOLINT + return std::async(std::launch::async, + // for this lambda you need C++14 + [this]() mutable { + absl::MutexLock::MutexLock(&mutex_.GetMutex()); + return true; + }); } void PostEvent() { - std::lock_guard guard(mutex_.GetWindowsMutex()); + absl::MutexLock::MutexLock(&mutex_.GetMutex()); mutex_.Unlock(); } @@ -50,7 +49,7 @@ TEST_F(MutexTests, SuccessfulRecursiveCreation) { location::nearby::windows::Mutex::Mode::kRecursive); // Act - std::recursive_mutex& actual = mutex.GetWindowsRecursiveMutex(); + std::recursive_mutex& actual = mutex.GetRecursiveMutex(); // Assert ASSERT_TRUE(actual.native_handle() != nullptr); @@ -62,10 +61,10 @@ TEST_F(MutexTests, SuccessfulCreation) { location::nearby::windows::Mutex::Mode::kRegular); // Act - std::mutex& actual = mutex.GetWindowsMutex(); + absl::Mutex& actual = mutex.GetMutex(); // Assert - ASSERT_TRUE(actual.native_handle() != nullptr); + ASSERT_TRUE(&actual != nullptr); } TEST_F(MutexTests, SuccessfulSignal) { diff --git a/internal/platform/implementation/windows/platform.cc b/internal/platform/implementation/windows/platform.cc index 236f3477..c98dfbd2 100644 --- a/internal/platform/implementation/windows/platform.cc +++ b/internal/platform/implementation/windows/platform.cc @@ -98,13 +98,13 @@ ImplementationPlatform::CreateConditionVariable(Mutex* mutex) { std::unique_ptr ImplementationPlatform::CreateInputFile( PayloadId payload_id, std::int64_t total_size) { - return absl::make_unique(GetPayloadPath(payload_id), - total_size); + return shared::IOFile::CreateInputFile(GetPayloadPath(payload_id), + total_size); } std::unique_ptr ImplementationPlatform::CreateOutputFile( PayloadId payload_id) { - return absl::make_unique(GetPayloadPath(payload_id)); + return shared::IOFile::CreateOutputFile(GetPayloadPath(payload_id)); } // TODO(b/184975123): replace with real implementation. diff --git a/internal/platform/implementation/windows/test_utils.cc b/internal/platform/implementation/windows/test_utils.cc index 90c12f2f..71cb79ab 100644 --- a/internal/platform/implementation/windows/test_utils.cc +++ b/internal/platform/implementation/windows/test_utils.cc @@ -48,10 +48,14 @@ std::string GetPayloadPath(location::nearby::PayloadId payload_id) { // is no longer needed by calling CoTaskMemFree, whether // SHGetKnownFolderPath succeeds or not. - char* fullpathUTF8 = new char((wcslen(basePath) + 1) * sizeof(char)); - wcstombs(fullpathUTF8, basePath, (wcslen(basePath) + 1) * sizeof(char)); + size_t bufferSize; + wcstombs_s(&bufferSize, NULL, 0, basePath, 0); + char* fullpathUTF8 = new char[bufferSize + 1]; + memset(fullpathUTF8, 0, bufferSize); + wcstombs_s(&bufferSize, fullpathUTF8, bufferSize, basePath, bufferSize - 1); std::string fullPath = std::string(fullpathUTF8); - auto retval = absl::StrCat(fullPath += "/", payload_id); + auto retval = absl::StrCat(fullPath += "\\", payload_id); + delete[] fullpathUTF8; return retval; } } // namespace test_utils