From 32b09538bad836514564b112393e13c92d0f08b3 Mon Sep 17 00:00:00 2001 From: Mateusz Karbowiak <69864511+mtszkarbowiak@users.noreply.github.com> Date: Sat, 5 Oct 2024 21:21:51 +0200 Subject: [PATCH 01/14] Hard `Nullable` refactor * --- Source/Engine/Core/Types/Nullable.h | 316 ++++++++++++++++++++-------- 1 file changed, 226 insertions(+), 90 deletions(-) diff --git a/Source/Engine/Core/Types/Nullable.h b/Source/Engine/Core/Types/Nullable.h index 9511f0df2..f291eb0dc 100644 --- a/Source/Engine/Core/Types/Nullable.h +++ b/Source/Engine/Core/Types/Nullable.h @@ -8,39 +8,132 @@ /// Represents a value type that can be assigned null. A nullable type can represent the correct range of values for its underlying value type, plus an additional null value. /// template -struct NullableBase +struct Nullable { -protected: +private: + union // Prevents default construction of T + { + T _value; + }; bool _hasValue; - T _value; + + /// + /// Ensures that the lifetime of the wrapped value ends correctly. This method is called when the state of the wrapper is no more needed. + /// + FORCE_INLINE void KillOld() + { + if (_hasValue) + { + _value.~T(); + } + } public: /// - /// Initializes a new instance of the struct. + /// Initializes a new instance of the struct with a null value. /// - NullableBase() + Nullable() + : _hasValue(false) { - _hasValue = false; + // Value is not initialized. + } + + ~Nullable() + { + KillOld(); } /// - /// Initializes a new instance of the struct. + /// Initializes a new instance of the struct by copying the value. /// - /// The initial value. - NullableBase(const T& value) + /// The initial wrapped value. + Nullable(const T& value) + : _value(value) + , _hasValue(true) { - _value = value; - _hasValue = true; } /// - /// Initializes a new instance of the struct. + /// Initializes a new instance of the struct by moving the value. /// - /// The other. - NullableBase(const NullableBase& other) + /// The initial wrapped value. + Nullable(T&& value) noexcept + : _value(MoveTemp(value)) + , _hasValue(true) + { + } + + /// + /// Initializes a new instance of the struct by copying the value from another instance. + /// + /// The wrapped value to be copied. + Nullable(const Nullable& other) + : _value(other._value) + , _hasValue(other._hasValue) + { + } + + /// + /// Initializes a new instance of the struct by moving the value from another instance. + /// + /// The wrapped value to be moved. + Nullable(Nullable&& other) noexcept { - _value = other._value; _hasValue = other._hasValue; + if (_hasValue) + { + new (&_value) T(MoveTemp(other._value)); // Placement new (move constructor) + } + + other.Reset(); + } + + auto operator=(const T& value) -> Nullable& + { + KillOld(); + + new (&_value) T(value); // Placement new (copy constructor) + _hasValue = true; + + return *this; + } + + auto operator=(T&& value) noexcept -> Nullable& + { + KillOld(); + + new (&_value) T(MoveTemp(value)); // Placement new (move constructor) + _hasValue = true; + + return *this; + } + + auto operator=(const Nullable& other) -> Nullable& + { + KillOld(); + + _hasValue = other._hasValue; + if (_hasValue) + { + new (&_value) T(other._value); // Placement new (copy constructor) + } + + return *this; + } + + auto operator=(Nullable&& other) noexcept -> Nullable& + { + KillOld(); + + _hasValue = other._hasValue; + if (_hasValue) + { + new (&_value) T(MoveTemp(other._value)); // Placement new (move constructor) + + other.Reset(); + } + + return *this; } public: @@ -64,30 +157,61 @@ public: } /// - /// Gets the value of the current NullableBase{T} object if it has been assigned a valid underlying value. + /// Gets a reference to the value of the current NullableBase{T} object. + /// If is assumed that the value is valid, otherwise the behavior is undefined. /// - /// The value. - FORCE_INLINE T GetValue() + /// In the past, this value returned a copy of the stored value. Be careful. + /// Reference to the value. + FORCE_INLINE T& GetValue() { ASSERT(_hasValue); return _value; } /// - /// Sets the value. + /// Sets the wrapped value. /// - /// The value. - void SetValue(const T& value) + /// The value to be copied. + FORCE_INLINE void SetValue(const T& value) { - _value = value; + if (_hasValue) + { + _value.~T(); + } + + new (&_value) T(value); // Placement new (copy constructor) + _hasValue = true; + } + + /// + /// Sets the wrapped value. + /// + /// The value to be moved. + FORCE_INLINE void SetValue(T&& value) noexcept + { + KillOld(); + + new (&_value) T(MoveTemp(value)); // Placement new (move constructor) _hasValue = true; } /// /// Resets the value. /// - void Reset() + FORCE_INLINE void Reset() { + KillOld(); + _hasValue = false; + } + + /// + /// Moves the value from the current NullableBase{T} object and resets it. + /// + FORCE_INLINE void GetAndReset(T& value) + { + ASSERT(_hasValue); + value = MoveTemp(_value); + _value.~T(); // Move is not destructive. _hasValue = false; } @@ -97,14 +221,14 @@ public: /// /// The other object. /// True if both values are equal. - bool operator==(const NullableBase& other) const + FORCE_INLINE bool operator==(const Nullable& other) const { - if (_hasValue) + if (other._hasValue != _hasValue) { - return other._hasValue && _value == other._value; + return false; } - return !other._hasValue; + return _value == other._value; } /// @@ -112,43 +236,19 @@ public: /// /// The other object. /// True if both values are not equal. - FORCE_INLINE bool operator!=(const NullableBase& other) const + FORCE_INLINE bool operator!=(const Nullable& other) const { return !operator==(other); } -}; - -/// -/// Represents a value type that can be assigned null. A nullable type can represent the correct range of values for its underlying value type, plus an additional null value. -/// -template -struct Nullable : NullableBase -{ -public: - /// - /// Initializes a new instance of the struct. - /// - Nullable() - : NullableBase() - { - } /// - /// Initializes a new instance of the struct. + /// Explicit conversion to boolean value. /// - /// The initial value. - Nullable(const T& value) - : NullableBase(value) - { - } - - /// - /// Initializes a new instance of the struct. - /// - /// The other. - Nullable(const Nullable& other) - : NullableBase(other) + /// True if this object has a valid value, otherwise false + /// Hint: If-statements are able to use explicit cast implicitly (sic). + FORCE_INLINE explicit operator bool() const { + return _hasValue; } }; @@ -156,43 +256,81 @@ public: /// Nullable value container that contains a boolean value or null. /// template<> -struct Nullable : NullableBase +struct Nullable { -public: - /// - /// Initializes a new instance of the struct. - /// - Nullable() - : NullableBase() +private: + enum class Value : uint8 { - } + False = 0, + True = 1, + Null = 2, + }; - /// - /// Initializes a new instance of the struct. - /// - /// The initial value. - Nullable(bool value) - : NullableBase(value) - { - } - - /// - /// Initializes a new instance of the struct. - /// - /// The other. - Nullable(const Nullable& other) - : NullableBase(other) - { - } + Value _value = Value::Null; public: + Nullable() = default; + + ~Nullable() = default; + + + Nullable(Nullable&& value) = default; + + Nullable(const Nullable& value) = default; + + Nullable(const bool value) noexcept + { + _value = value ? Value::True : Value::False; + } + + + auto operator=(const bool value) noexcept -> Nullable& + { + _value = value ? Value::True : Value::False; + return *this; + } + + auto operator=(const Nullable& value) -> Nullable& = default; + + auto operator=(Nullable&& value) -> Nullable& = default; + + + FORCE_INLINE bool HasValue() const noexcept + { + return _value != Value::Null; + } + + FORCE_INLINE bool GetValue() const + { + ASSERT(_value != Value::Null); + return _value == Value::True; + } + + FORCE_INLINE void SetValue(const bool value) noexcept + { + _value = value ? Value::True : Value::False; + } + + FORCE_INLINE void Reset() noexcept + { + _value = Value::Null; + } + + FORCE_INLINE void GetAndReset(bool& value) noexcept + { + ASSERT(_value != Value::Null); + value = _value == Value::True; + _value = Value::Null; + } + + /// /// Gets a value indicating whether the current Nullable{T} object has a valid value and it's set to true. /// /// true if this object has a valid value set to true; otherwise, false. FORCE_INLINE bool IsTrue() const { - return _hasValue && _value; + return _value == Value::True; } /// @@ -201,15 +339,13 @@ public: /// true if this object has a valid value set to false; otherwise, false. FORCE_INLINE bool IsFalse() const { - return _hasValue && !_value; + return _value == Value::False; } /// - /// Implicit conversion to boolean value. + /// Getting if provoke unacceptably ambiguous code. For template meta-programming use explicit HasValue() instead. /// - /// True if this object has a valid value set to true, otherwise false - FORCE_INLINE operator bool() const - { - return _hasValue && _value; - } + explicit operator bool() const = delete; + + // Note: Even though IsTrue and IsFalse have been added for convenience, but they may be used for performance reasons. }; From db06f4f72ec87002d095bf8994120cf75f862d0c Mon Sep 17 00:00:00 2001 From: Mateusz Karbowiak <69864511+mtszkarbowiak@users.noreply.github.com> Date: Sat, 5 Oct 2024 21:22:36 +0200 Subject: [PATCH 02/14] Fixed implicit type conversion for type specialization --- Source/Editor/Editor.cpp | 8 ++++---- Source/Editor/Windows/SplashScreen.cpp | 2 +- Source/Engine/Core/Log.cpp | 2 +- Source/Engine/Engine/Engine.cpp | 4 ++-- Source/Engine/Graphics/Graphics.cpp | 12 ++++++------ .../Graphics/Shaders/Cache/ShaderAssetBase.cpp | 4 ++-- .../Graphics/Shaders/Cache/ShaderCacheManager.cpp | 4 ++-- .../GraphicsDevice/DirectX/DX11/GPUDeviceDX11.cpp | 10 +++++----- .../GraphicsDevice/DirectX/DX12/GPUDeviceDX12.cpp | 8 ++++---- Source/Engine/Platform/Base/PlatformBase.cpp | 6 +++--- Source/Engine/Platform/Windows/WindowsPlatform.cpp | 2 +- 11 files changed, 31 insertions(+), 31 deletions(-) diff --git a/Source/Editor/Editor.cpp b/Source/Editor/Editor.cpp index d0bfe2765..8fd646da9 100644 --- a/Source/Editor/Editor.cpp +++ b/Source/Editor/Editor.cpp @@ -403,7 +403,7 @@ int32 Editor::LoadProduct() } // Create new project option - if (CommandLine::Options.NewProject) + if (CommandLine::Options.NewProject.IsTrue()) { Array projectFiles; FileSystem::DirectoryGetFiles(projectFiles, projectPath, TEXT("*.flaxproj"), DirectorySearchOption::TopDirectoryOnly); @@ -428,7 +428,7 @@ int32 Editor::LoadProduct() } } } - if (CommandLine::Options.NewProject) + if (CommandLine::Options.NewProject.IsTrue()) { if (projectPath.IsEmpty()) projectPath = Platform::GetWorkingDirectory(); @@ -529,7 +529,7 @@ int32 Editor::LoadProduct() if (projectPath.IsEmpty()) { #if PLATFORM_HAS_HEADLESS_MODE - if (CommandLine::Options.Headless) + if (CommandLine::Options.Headless.IsTrue()) { Platform::Fatal(TEXT("Missing project path.")); return -1; @@ -657,7 +657,7 @@ Window* Editor::CreateMainWindow() bool Editor::Init() { // Scripts project files generation from command line - if (CommandLine::Options.GenProjectFiles) + if (CommandLine::Options.GenProjectFiles.IsTrue()) { const String customArgs = TEXT("-verbose -log -logfile=\"Cache/Intermediate/ProjectFileLog.txt\""); const bool failed = ScriptsBuilder::GenerateProject(customArgs); diff --git a/Source/Editor/Windows/SplashScreen.cpp b/Source/Editor/Windows/SplashScreen.cpp index 49257d281..39f7691e7 100644 --- a/Source/Editor/Windows/SplashScreen.cpp +++ b/Source/Editor/Windows/SplashScreen.cpp @@ -147,7 +147,7 @@ SplashScreen::~SplashScreen() void SplashScreen::Show() { // Skip if already shown or in headless mode - if (IsVisible() || CommandLine::Options.Headless) + if (IsVisible() || CommandLine::Options.Headless.IsTrue()) return; LOG(Info, "Showing splash screen"); diff --git a/Source/Engine/Core/Log.cpp b/Source/Engine/Core/Log.cpp index c8cf4419e..38d591c51 100644 --- a/Source/Engine/Core/Log.cpp +++ b/Source/Engine/Core/Log.cpp @@ -119,7 +119,7 @@ void Log::Logger::Write(const StringView& msg) IsDuringLog = true; // Send message to standard process output - if (CommandLine::Options.Std) + if (CommandLine::Options.Std.IsTrue()) { #if PLATFORM_TEXT_IS_CHAR16 StringAnsi ansi(msg); diff --git a/Source/Engine/Engine/Engine.cpp b/Source/Engine/Engine/Engine.cpp index b26cbd25a..7fea9cd6d 100644 --- a/Source/Engine/Engine/Engine.cpp +++ b/Source/Engine/Engine/Engine.cpp @@ -631,9 +631,9 @@ void EngineImpl::InitPaths() FileSystem::CreateDirectory(Globals::ProjectContentFolder); if (!FileSystem::DirectoryExists(Globals::ProjectSourceFolder)) FileSystem::CreateDirectory(Globals::ProjectSourceFolder); - if (CommandLine::Options.ClearCache) + if (CommandLine::Options.ClearCache.IsTrue()) FileSystem::DeleteDirectory(Globals::ProjectCacheFolder, true); - else if (CommandLine::Options.ClearCookerCache) + else if (CommandLine::Options.ClearCookerCache.IsTrue()) FileSystem::DeleteDirectory(Globals::ProjectCacheFolder / TEXT("Cooker"), true); if (!FileSystem::DirectoryExists(Globals::ProjectCacheFolder)) FileSystem::CreateDirectory(Globals::ProjectCacheFolder); diff --git a/Source/Engine/Graphics/Graphics.cpp b/Source/Engine/Graphics/Graphics.cpp index a1640c0b4..25437d25d 100644 --- a/Source/Engine/Graphics/Graphics.cpp +++ b/Source/Engine/Graphics/Graphics.cpp @@ -104,7 +104,7 @@ bool GraphicsService::Init() GPUDevice* device = nullptr; // Null - if (!device && CommandLine::Options.Null) + if (!device && CommandLine::Options.Null.IsTrue()) { #if GRAPHICS_API_NULL device = CreateGPUDeviceNull(); @@ -114,7 +114,7 @@ bool GraphicsService::Init() } // Vulkan - if (!device && CommandLine::Options.Vulkan) + if (!device && CommandLine::Options.Vulkan.IsTrue()) { #if GRAPHICS_API_VULKAN device = CreateGPUDeviceVulkan(); @@ -124,7 +124,7 @@ bool GraphicsService::Init() } // DirectX 12 - if (!device && CommandLine::Options.D3D12) + if (!device && CommandLine::Options.D3D12.IsTrue()) { #if GRAPHICS_API_DIRECTX12 if (Platform::IsWindows10()) @@ -137,7 +137,7 @@ bool GraphicsService::Init() } // DirectX 11 and DirectX 10 - if (!device && (CommandLine::Options.D3D11 || CommandLine::Options.D3D10)) + if (!device && (CommandLine::Options.D3D11.IsTrue() || CommandLine::Options.D3D10.IsTrue())) { #if GRAPHICS_API_DIRECTX11 device = CreateGPUDeviceDX11(); @@ -193,10 +193,10 @@ bool GraphicsService::Init() // Initialize if (device->IsDebugToolAttached #if USE_EDITOR || !BUILD_RELEASE - || CommandLine::Options.ShaderProfile + || CommandLine::Options.ShaderProfile.IsTrue() #endif #if USE_EDITOR - || CommandLine::Options.ShaderDebug + || CommandLine::Options.ShaderDebug.IsTrue() #endif ) { diff --git a/Source/Engine/Graphics/Shaders/Cache/ShaderAssetBase.cpp b/Source/Engine/Graphics/Shaders/Cache/ShaderAssetBase.cpp index 6f91ff4be..acc1dc7db 100644 --- a/Source/Engine/Graphics/Shaders/Cache/ShaderAssetBase.cpp +++ b/Source/Engine/Graphics/Shaders/Cache/ShaderAssetBase.cpp @@ -249,12 +249,12 @@ bool ShaderAssetBase::LoadShaderCache(ShaderCacheResult& result) options.SourceLength = sourceLength; options.Profile = shaderProfile; options.Output = &cacheStream; - if (CommandLine::Options.ShaderDebug) + if (CommandLine::Options.ShaderDebug.IsTrue()) { options.GenerateDebugData = true; options.NoOptimize = true; } - else if (CommandLine::Options.ShaderProfile) + else if (CommandLine::Options.ShaderProfile.IsTrue()) { options.GenerateDebugData = true; } diff --git a/Source/Engine/Graphics/Shaders/Cache/ShaderCacheManager.cpp b/Source/Engine/Graphics/Shaders/Cache/ShaderCacheManager.cpp index b8adce351..112324b2c 100644 --- a/Source/Engine/Graphics/Shaders/Cache/ShaderCacheManager.cpp +++ b/Source/Engine/Graphics/Shaders/Cache/ShaderCacheManager.cpp @@ -193,8 +193,8 @@ bool ShaderCacheManagerService::Init() CacheVersion cacheVersion; const String cacheVerFile = rootDir / TEXT("CacheVersion"); #if USE_EDITOR - const bool shaderDebug = CommandLine::Options.ShaderDebug; - const bool shaderProfile = CommandLine::Options.ShaderProfile; + const bool shaderDebug = CommandLine::Options.ShaderDebug.IsTrue(); + const bool shaderProfile = CommandLine::Options.ShaderProfile.IsTrue(); #else const bool shaderDebug = false; #endif diff --git a/Source/Engine/GraphicsDevice/DirectX/DX11/GPUDeviceDX11.cpp b/Source/Engine/GraphicsDevice/DirectX/DX11/GPUDeviceDX11.cpp index 432a44ee8..724a5ea71 100644 --- a/Source/Engine/GraphicsDevice/DirectX/DX11/GPUDeviceDX11.cpp +++ b/Source/Engine/GraphicsDevice/DirectX/DX11/GPUDeviceDX11.cpp @@ -106,9 +106,9 @@ GPUDevice* GPUDeviceDX11::Create() #else D3D_FEATURE_LEVEL maxAllowedFeatureLevel = D3D_FEATURE_LEVEL_11_0; #endif - if (CommandLine::Options.D3D10) + if (CommandLine::Options.D3D10.IsTrue()) maxAllowedFeatureLevel = D3D_FEATURE_LEVEL_10_0; - else if (CommandLine::Options.D3D11) + else if (CommandLine::Options.D3D11.IsTrue()) maxAllowedFeatureLevel = D3D_FEATURE_LEVEL_11_0; #if !USE_EDITOR && PLATFORM_WINDOWS auto winSettings = WindowsPlatformSettings::Get(); @@ -209,11 +209,11 @@ GPUDevice* GPUDeviceDX11::Create() } GPUAdapterDX selectedAdapter = adapters[selectedAdapterIndex]; uint32 vendorId = 0; - if (CommandLine::Options.NVIDIA) + if (CommandLine::Options.NVIDIA.IsTrue()) vendorId = GPU_VENDOR_ID_NVIDIA; - else if (CommandLine::Options.AMD) + else if (CommandLine::Options.AMD.IsTrue()) vendorId = GPU_VENDOR_ID_AMD; - else if (CommandLine::Options.Intel) + else if (CommandLine::Options.Intel.IsTrue()) vendorId = GPU_VENDOR_ID_INTEL; if (vendorId != 0) { diff --git a/Source/Engine/GraphicsDevice/DirectX/DX12/GPUDeviceDX12.cpp b/Source/Engine/GraphicsDevice/DirectX/DX12/GPUDeviceDX12.cpp index a33cd8194..78c047cfc 100644 --- a/Source/Engine/GraphicsDevice/DirectX/DX12/GPUDeviceDX12.cpp +++ b/Source/Engine/GraphicsDevice/DirectX/DX12/GPUDeviceDX12.cpp @@ -161,11 +161,11 @@ GPUDevice* GPUDeviceDX12::Create() } GPUAdapterDX selectedAdapter = adapters[selectedAdapterIndex]; uint32 vendorId = 0; - if (CommandLine::Options.NVIDIA) + if (CommandLine::Options.NVIDIA.IsTrue()) vendorId = GPU_VENDOR_ID_NVIDIA; - else if (CommandLine::Options.AMD) + else if (CommandLine::Options.AMD.IsTrue()) vendorId = GPU_VENDOR_ID_AMD; - else if (CommandLine::Options.Intel) + else if (CommandLine::Options.Intel.IsTrue()) vendorId = GPU_VENDOR_ID_INTEL; if (vendorId != 0) { @@ -425,7 +425,7 @@ bool GPUDeviceDX12::Init() #if !BUILD_RELEASE // Prevent the GPU from overclocking or under-clocking to get consistent timings - if (CommandLine::Options.ShaderProfile) + if (CommandLine::Options.ShaderProfile.IsTrue()) { _device->SetStablePowerState(TRUE); } diff --git a/Source/Engine/Platform/Base/PlatformBase.cpp b/Source/Engine/Platform/Base/PlatformBase.cpp index 6efa3c8b4..93b06b821 100644 --- a/Source/Engine/Platform/Base/PlatformBase.cpp +++ b/Source/Engine/Platform/Base/PlatformBase.cpp @@ -365,7 +365,7 @@ void PlatformBase::Fatal(const Char* msg, void* context) void PlatformBase::Error(const Char* msg) { #if PLATFORM_HAS_HEADLESS_MODE - if (CommandLine::Options.Headless) + if (CommandLine::Options.Headless.IsTrue()) { #if PLATFORM_TEXT_IS_CHAR16 StringAnsi ansi(msg); @@ -385,7 +385,7 @@ void PlatformBase::Error(const Char* msg) void PlatformBase::Warning(const Char* msg) { #if PLATFORM_HAS_HEADLESS_MODE - if (CommandLine::Options.Headless) + if (CommandLine::Options.Headless.IsTrue()) { std::cout << "Warning: " << msg << std::endl; } @@ -399,7 +399,7 @@ void PlatformBase::Warning(const Char* msg) void PlatformBase::Info(const Char* msg) { #if PLATFORM_HAS_HEADLESS_MODE - if (CommandLine::Options.Headless) + if (CommandLine::Options.Headless.IsTrue()) { std::cout << "Info: " << msg << std::endl; } diff --git a/Source/Engine/Platform/Windows/WindowsPlatform.cpp b/Source/Engine/Platform/Windows/WindowsPlatform.cpp index 0f1159fc5..57d46b1f8 100644 --- a/Source/Engine/Platform/Windows/WindowsPlatform.cpp +++ b/Source/Engine/Platform/Windows/WindowsPlatform.cpp @@ -616,7 +616,7 @@ bool WindowsPlatform::Init() return true; // Init console output (engine is linked with /SUBSYSTEM:WINDOWS so it lacks of proper console output on Windows) - if (CommandLine::Options.Std) + if (CommandLine::Options.Std.IsTrue()) { // Attaches output of application to parent console, returns true if running in console-mode // [Reference: https://www.tillett.info/2013/05/13/how-to-create-a-windows-program-that-works-as-both-as-a-gui-and-console-application] From edfbeea0e696c0ae402643670a0b20f6f51b447e Mon Sep 17 00:00:00 2001 From: Mateusz Karbowiak <69864511+mtszkarbowiak@users.noreply.github.com> Date: Sat, 5 Oct 2024 21:46:30 +0200 Subject: [PATCH 03/14] `Nullable` utility functions --- Source/Engine/Core/Types/Nullable.h | 73 +++++++++++++++++++++++++---- 1 file changed, 63 insertions(+), 10 deletions(-) diff --git a/Source/Engine/Core/Types/Nullable.h b/Source/Engine/Core/Types/Nullable.h index f291eb0dc..fe6db7e4a 100644 --- a/Source/Engine/Core/Types/Nullable.h +++ b/Source/Engine/Core/Types/Nullable.h @@ -80,6 +80,7 @@ public: Nullable(Nullable&& other) noexcept { _hasValue = other._hasValue; + if (_hasValue) { new (&_value) T(MoveTemp(other._value)); // Placement new (move constructor) @@ -112,31 +113,35 @@ public: { KillOld(); - _hasValue = other._hasValue; - if (_hasValue) + if (other._hasValue) { new (&_value) T(other._value); // Placement new (copy constructor) } + _hasValue = other._hasValue; // Set the flag AFTER the value is copied. return *this; } auto operator=(Nullable&& other) noexcept -> Nullable& { + if (this == &other) + { + return *this; + } + KillOld(); - _hasValue = other._hasValue; if (_hasValue) { new (&_value) T(MoveTemp(other._value)); // Placement new (move constructor) other.Reset(); } + _hasValue = other._hasValue; // Set the flag AFTER the value is moved. return *this; } -public: /// /// Gets a value indicating whether the current NullableBase{T} object has a valid value of its underlying type. /// @@ -168,6 +173,16 @@ public: return _value; } + FORCE_INLINE const T& GetValueOr(const T& defaultValue) const + { + return _hasValue ? _value : defaultValue; + } + + FORCE_INLINE T GetValueOr(T&& defaultValue) const noexcept + { + return _hasValue ? _value : defaultValue; + } + /// /// Sets the wrapped value. /// @@ -180,7 +195,7 @@ public: } new (&_value) T(value); // Placement new (copy constructor) - _hasValue = true; + _hasValue = true; // Set the flag AFTER the value is copied. } /// @@ -192,7 +207,31 @@ public: KillOld(); new (&_value) T(MoveTemp(value)); // Placement new (move constructor) - _hasValue = true; + _hasValue = true; // Set the flag AFTER the value is moved. + } + + FORCE_INLINE bool TrySet(T&& value) noexcept + { + if (_hasValue) + { + return false; + } + + new (&_value) T(MoveTemp(value)); // Placement new (move constructor) + _hasValue = true; // Set the flag AFTER the value is moved. + return true; + } + + FORCE_INLINE bool TrySet(const T& value) + { + if (_hasValue) + { + return false; + } + + new (&_value) T(value); // Placement new (copy constructor) + _hasValue = true; // Set the flag AFTER the value is copied. + return true; } /// @@ -201,7 +240,7 @@ public: FORCE_INLINE void Reset() { KillOld(); - _hasValue = false; + _hasValue = false; // Reset the flag AFTER the value is (potentially) destructed. } /// @@ -211,11 +250,9 @@ public: { ASSERT(_hasValue); value = MoveTemp(_value); - _value.~T(); // Move is not destructive. - _hasValue = false; + Reset(); } -public: /// /// Indicates whether the current NullableBase{T} object is equal to a specified object. /// @@ -306,11 +343,27 @@ public: return _value == Value::True; } + FORCE_INLINE bool GetValueOr(const bool defaultValue) const noexcept + { + return _value == Value::Null ? defaultValue : _value == Value::True; + } + FORCE_INLINE void SetValue(const bool value) noexcept { _value = value ? Value::True : Value::False; } + FORCE_INLINE bool TrySet(const bool value) noexcept + { + if (_value != Value::Null) + { + return false; + } + + _value = value ? Value::True : Value::False; + return true; + } + FORCE_INLINE void Reset() noexcept { _value = Value::Null; From a2874a189e207fdfa5942ebffa0cfe28921a8d46 Mon Sep 17 00:00:00 2001 From: Mateusz Karbowiak <69864511+mtszkarbowiak@users.noreply.github.com> Date: Sat, 5 Oct 2024 23:51:54 +0200 Subject: [PATCH 04/14] `Nullable` docs --- Source/Engine/Core/Types/Nullable.h | 174 +++++++++++++++++++--------- 1 file changed, 122 insertions(+), 52 deletions(-) diff --git a/Source/Engine/Core/Types/Nullable.h b/Source/Engine/Core/Types/Nullable.h index fe6db7e4a..655233b85 100644 --- a/Source/Engine/Core/Types/Nullable.h +++ b/Source/Engine/Core/Types/Nullable.h @@ -5,20 +5,20 @@ #include "Engine/Platform/Platform.h" /// -/// Represents a value type that can be assigned null. A nullable type can represent the correct range of values for its underlying value type, plus an additional null value. +/// Wrapper for a value type that can be assigned null, controlling the lifetime of the wrapped value. /// template struct Nullable { private: - union // Prevents default construction of T + union { T _value; }; bool _hasValue; /// - /// Ensures that the lifetime of the wrapped value ends correctly. This method is called when the state of the wrapper is no more needed. + /// Ends the lifetime of the wrapped value by calling its destructor, if the lifetime has not ended yet. Otherwise, does nothing. /// FORCE_INLINE void KillOld() { @@ -30,7 +30,7 @@ private: public: /// - /// Initializes a new instance of the struct with a null value. + /// Initializes by setting the wrapped value to null. /// Nullable() : _hasValue(false) @@ -44,9 +44,9 @@ public: } /// - /// Initializes a new instance of the struct by copying the value. + /// Initializes by copying the wrapped value. /// - /// The initial wrapped value. + /// The initial wrapped value to be copied. Nullable(const T& value) : _value(value) , _hasValue(true) @@ -54,9 +54,9 @@ public: } /// - /// Initializes a new instance of the struct by moving the value. + /// Initializes by moving the wrapped value. /// - /// The initial wrapped value. + /// The initial wrapped value to be moved. Nullable(T&& value) noexcept : _value(MoveTemp(value)) , _hasValue(true) @@ -64,7 +64,7 @@ public: } /// - /// Initializes a new instance of the struct by copying the value from another instance. + /// Initializes by copying another . /// /// The wrapped value to be copied. Nullable(const Nullable& other) @@ -74,7 +74,7 @@ public: } /// - /// Initializes a new instance of the struct by moving the value from another instance. + /// Initializes by moving another . /// /// The wrapped value to be moved. Nullable(Nullable&& other) noexcept @@ -89,6 +89,9 @@ public: other.Reset(); } + /// + /// Reassigns the wrapped value by copying. + /// auto operator=(const T& value) -> Nullable& { KillOld(); @@ -99,6 +102,9 @@ public: return *this; } + /// + /// Reassigns the wrapped value by moving. + /// auto operator=(T&& value) noexcept -> Nullable& { KillOld(); @@ -109,6 +115,9 @@ public: return *this; } + /// + /// Reassigns the wrapped value by copying another . + /// auto operator=(const Nullable& other) -> Nullable& { KillOld(); @@ -122,6 +131,9 @@ public: return *this; } + /// + /// Reassigns the wrapped value by moving another . + /// auto operator=(Nullable&& other) noexcept -> Nullable& { if (this == &other) @@ -143,7 +155,7 @@ public: } /// - /// Gets a value indicating whether the current NullableBase{T} object has a valid value of its underlying type. + /// Checks if wrapped object has a valid value. /// /// true if this object has a valid value; otherwise, false. FORCE_INLINE bool HasValue() const @@ -152,9 +164,9 @@ public: } /// - /// Gets the value of the current NullableBase{T} object if it has been assigned a valid underlying value. + /// Gets a const reference to the wrapped value. If the value is not valid, the behavior is undefined. /// - /// The value. + /// Reference to the wrapped value. FORCE_INLINE const T& GetValue() const { ASSERT(_hasValue); @@ -162,29 +174,36 @@ public: } /// - /// Gets a reference to the value of the current NullableBase{T} object. - /// If is assumed that the value is valid, otherwise the behavior is undefined. + /// Gets a reference to the wrapped value. If the value is not valid, the behavior is undefined. + /// This method can be used to reassign the wrapped value. /// - /// In the past, this value returned a copy of the stored value. Be careful. - /// Reference to the value. + /// Reference to the wrapped value. FORCE_INLINE T& GetValue() { ASSERT(_hasValue); return _value; } + /// + /// Gets a const reference to the wrapped value or a default value if the value is not valid. + /// + /// Reference to the wrapped value or the default value. FORCE_INLINE const T& GetValueOr(const T& defaultValue) const { return _hasValue ? _value : defaultValue; } + /// + /// Gets an instance of the wrapped value or a default value based on r-value reference, if the wrapped value is not valid. + /// + /// Copy of the wrapped value or the default value. FORCE_INLINE T GetValueOr(T&& defaultValue) const noexcept { return _hasValue ? _value : defaultValue; } /// - /// Sets the wrapped value. + /// Sets the wrapped value by copying. /// /// The value to be copied. FORCE_INLINE void SetValue(const T& value) @@ -199,7 +218,7 @@ public: } /// - /// Sets the wrapped value. + /// Sets the wrapped value by moving. /// /// The value to be moved. FORCE_INLINE void SetValue(T&& value) noexcept @@ -210,18 +229,10 @@ public: _hasValue = true; // Set the flag AFTER the value is moved. } - FORCE_INLINE bool TrySet(T&& value) noexcept - { - if (_hasValue) - { - return false; - } - - new (&_value) T(MoveTemp(value)); // Placement new (move constructor) - _hasValue = true; // Set the flag AFTER the value is moved. - return true; - } - + /// + /// If the wrapped value is not valid, sets it by copying. Otherwise, does nothing. + /// + /// True if the wrapped value was changed, otherwise false. FORCE_INLINE bool TrySet(const T& value) { if (_hasValue) @@ -235,7 +246,23 @@ public: } /// - /// Resets the value. + /// If the wrapped value is not valid, sets it by moving. Otherwise, does nothing. + /// + /// True if the wrapped value was changed, otherwise false. + FORCE_INLINE bool TrySet(T&& value) noexcept + { + if (_hasValue) + { + return false; + } + + new (&_value) T(MoveTemp(value)); // Placement new (move constructor) + _hasValue = true; // Set the flag AFTER the value is moved. + return true; + } + + /// + /// Disposes the wrapped value and sets the wrapped value to null. If the wrapped value is not valid, does nothing. /// FORCE_INLINE void Reset() { @@ -244,8 +271,9 @@ public: } /// - /// Moves the value from the current NullableBase{T} object and resets it. + /// Moves the wrapped value to the output parameter and sets the wrapped value to null. If the wrapped value is not valid, the behavior is undefined. /// + /// The output parameter that will receive the wrapped value. FORCE_INLINE void GetAndReset(T& value) { ASSERT(_hasValue); @@ -254,10 +282,10 @@ public: } /// - /// Indicates whether the current NullableBase{T} object is equal to a specified object. + /// Indicates whether this instance is equal to other one. /// /// The other object. - /// True if both values are equal. + /// true if both values are equal. FORCE_INLINE bool operator==(const Nullable& other) const { if (other._hasValue != _hasValue) @@ -269,20 +297,19 @@ public: } /// - /// Indicates whether the current NullableBase{T} object is not equal to a specified object. + /// Indicates whether this instance is NOT equal to other one. /// /// The other object. - /// True if both values are not equal. + /// true if both values are not equal. FORCE_INLINE bool operator!=(const Nullable& other) const { return !operator==(other); } /// - /// Explicit conversion to boolean value. + /// Explicit conversion to boolean value. Allows to check if the wrapped value is valid in if-statements without casting. /// - /// True if this object has a valid value, otherwise false - /// Hint: If-statements are able to use explicit cast implicitly (sic). + /// true if this object has a valid value, otherwise false FORCE_INLINE explicit operator bool() const { return _hasValue; @@ -290,69 +317,107 @@ public: }; /// -/// Nullable value container that contains a boolean value or null. +/// Specialization of for type. /// template<> struct Nullable { private: + /// + /// Underlying value of the nullable boolean. Uses only one byte to optimize memory usage. + /// enum class Value : uint8 { - False = 0, - True = 1, - Null = 2, + Null, + False, + True, }; Value _value = Value::Null; public: + /// + /// Initializes nullable boolean by setting the wrapped value to null. + /// Nullable() = default; ~Nullable() = default; - + /// + /// Initializes nullable boolean by moving another nullable boolean. + /// Nullable(Nullable&& value) = default; + /// + /// Initializes nullable boolean by copying another nullable boolean. + /// Nullable(const Nullable& value) = default; + /// + /// Initializes nullable boolean by implicitly casting a boolean value. + /// Nullable(const bool value) noexcept { _value = value ? Value::True : Value::False; } + /// + /// Reassigns the wrapped value by implicitly casting a boolean value. + /// auto operator=(const bool value) noexcept -> Nullable& { _value = value ? Value::True : Value::False; return *this; } + /// + /// Reassigns the wrapped value by copying another nullable boolean. + /// auto operator=(const Nullable& value) -> Nullable& = default; + /// + /// Reassigns the wrapped value by moving another nullable boolean. + /// auto operator=(Nullable&& value) -> Nullable& = default; + /// + /// Checks if wrapped bool has a valid value. + /// FORCE_INLINE bool HasValue() const noexcept { return _value != Value::Null; } + /// + /// Gets the wrapped boolean value. If the value is not valid, the behavior is undefined. + /// FORCE_INLINE bool GetValue() const { ASSERT(_value != Value::Null); return _value == Value::True; } + /// + /// Gets the wrapped boolean value. If the value is not valid, returns the default value. + /// FORCE_INLINE bool GetValueOr(const bool defaultValue) const noexcept { return _value == Value::Null ? defaultValue : _value == Value::True; } + /// + /// Sets the wrapped value to a valid boolean. + /// FORCE_INLINE void SetValue(const bool value) noexcept { _value = value ? Value::True : Value::False; } + /// + /// If the wrapped value is not valid, sets it to a valid boolean. + /// FORCE_INLINE bool TrySet(const bool value) noexcept { if (_value != Value::Null) @@ -364,11 +429,17 @@ public: return true; } + /// + /// Sets the wrapped bool to null. + /// FORCE_INLINE void Reset() noexcept { _value = Value::Null; } + /// + /// Moves the wrapped value to the output parameter and sets the wrapped value to null. If the wrapped value is not valid, the behavior is undefined. + /// FORCE_INLINE void GetAndReset(bool& value) noexcept { ASSERT(_value != Value::Null); @@ -378,27 +449,26 @@ public: /// - /// Gets a value indicating whether the current Nullable{T} object has a valid value and it's set to true. + /// Checks if the current object has a valid value and it's set to true. If the value is false or not valid, the method returns false. /// - /// true if this object has a valid value set to true; otherwise, false. FORCE_INLINE bool IsTrue() const { return _value == Value::True; } /// - /// Gets a value indicating whether the current Nullable{T} object has a valid value and it's set to false. + /// Checks if the current object has a valid value and it's set to false. If the value is true or not valid, the method returns false. /// - /// true if this object has a valid value set to false; otherwise, false. FORCE_INLINE bool IsFalse() const { return _value == Value::False; } /// - /// Getting if provoke unacceptably ambiguous code. For template meta-programming use explicit HasValue() instead. + /// Deletes implicit conversion to bool to prevent ambiguous code. /// + /// + /// Implicit cast from nullable bool to a bool produces unacceptably ambiguous code. For template meta-programming use explicit HasValue instead. + /// explicit operator bool() const = delete; - - // Note: Even though IsTrue and IsFalse have been added for convenience, but they may be used for performance reasons. }; From c9b1f6f516c445734e932a51def14197e837f4f9 Mon Sep 17 00:00:00 2001 From: Mateusz Karbowiak <69864511+mtszkarbowiak@users.noreply.github.com> Date: Sun, 6 Oct 2024 01:57:12 +0200 Subject: [PATCH 05/14] `Nullable` fixes --- Source/Engine/Core/Types/Nullable.h | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/Source/Engine/Core/Types/Nullable.h b/Source/Engine/Core/Types/Nullable.h index 655233b85..0cf4b4361 100644 --- a/Source/Engine/Core/Types/Nullable.h +++ b/Source/Engine/Core/Types/Nullable.h @@ -79,12 +79,11 @@ public: /// The wrapped value to be moved. Nullable(Nullable&& other) noexcept { - _hasValue = other._hasValue; - - if (_hasValue) + if (other._hasValue) { new (&_value) T(MoveTemp(other._value)); // Placement new (move constructor) } + _hasValue = other._hasValue; other.Reset(); } @@ -194,10 +193,10 @@ public: } /// - /// Gets an instance of the wrapped value or a default value based on r-value reference, if the wrapped value is not valid. + /// Gets a mutable reference to the wrapped value or a default value if the value is not valid. /// - /// Copy of the wrapped value or the default value. - FORCE_INLINE T GetValueOr(T&& defaultValue) const noexcept + /// Reference to the wrapped value or the default value. + FORCE_INLINE T& GetValueOr(T& defaultValue) const { return _hasValue ? _value : defaultValue; } @@ -266,8 +265,8 @@ public: /// FORCE_INLINE void Reset() { + _hasValue = false; // Reset the flag BEFORE the value is (potentially) destructed. KillOld(); - _hasValue = false; // Reset the flag AFTER the value is (potentially) destructed. } /// From 23624aa7f8ae73866049e9ce663c2b96fdff5605 Mon Sep 17 00:00:00 2001 From: Mateusz Karbowiak <69864511+mtszkarbowiak@users.noreply.github.com> Date: Sun, 6 Oct 2024 02:23:21 +0200 Subject: [PATCH 06/14] Fix type constraints --- Source/Engine/Core/Types/Nullable.h | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/Source/Engine/Core/Types/Nullable.h b/Source/Engine/Core/Types/Nullable.h index 0cf4b4361..9d8f3f65b 100644 --- a/Source/Engine/Core/Types/Nullable.h +++ b/Source/Engine/Core/Types/Nullable.h @@ -7,13 +7,19 @@ /// /// Wrapper for a value type that can be assigned null, controlling the lifetime of the wrapped value. /// +/// +/// The type of the wrapped value. It must be move-constructible but does not have to be copy-constructible. Value is never reassigned. +/// template struct Nullable { private: + struct Dummy { Dummy() {} }; + union { - T _value; + T _value; + Dummy _dummy; }; bool _hasValue; @@ -28,12 +34,18 @@ private: } } + /// + /// true if the wrapped type is copy constructible. + /// + constexpr static bool IsCopyConstructible = TIsCopyConstructible::Value; + public: /// /// Initializes by setting the wrapped value to null. /// Nullable() - : _hasValue(false) + : _dummy() + , _hasValue(false) { // Value is not initialized. } @@ -47,6 +59,7 @@ public: /// Initializes by copying the wrapped value. /// /// The initial wrapped value to be copied. + template::Type> Nullable(const T& value) : _value(value) , _hasValue(true) @@ -67,6 +80,7 @@ public: /// Initializes by copying another . /// /// The wrapped value to be copied. + template::Type> Nullable(const Nullable& other) : _value(other._value) , _hasValue(other._hasValue) @@ -91,6 +105,7 @@ public: /// /// Reassigns the wrapped value by copying. /// + template::Type> auto operator=(const T& value) -> Nullable& { KillOld(); @@ -117,6 +132,7 @@ public: /// /// Reassigns the wrapped value by copying another . /// + template::Type> auto operator=(const Nullable& other) -> Nullable& { KillOld(); @@ -205,6 +221,7 @@ public: /// Sets the wrapped value by copying. /// /// The value to be copied. + template::Type> FORCE_INLINE void SetValue(const T& value) { if (_hasValue) @@ -232,6 +249,7 @@ public: /// If the wrapped value is not valid, sets it by copying. Otherwise, does nothing. /// /// True if the wrapped value was changed, otherwise false. + template::Type> FORCE_INLINE bool TrySet(const T& value) { if (_hasValue) From 6f6348508a1a5b16cf10ad24f0a3db26cec71da0 Mon Sep 17 00:00:00 2001 From: Mateusz Karbowiak <69864511+mtszkarbowiak@users.noreply.github.com> Date: Sun, 6 Oct 2024 02:53:11 +0200 Subject: [PATCH 07/14] `Nullable` implicit cast fix --- .../Engine/GraphicsDevice/Vulkan/GPUDeviceVulkan.cpp | 6 +++--- Source/Engine/Platform/Linux/LinuxPlatform.cpp | 10 +++++----- Source/Engine/Platform/Mac/MacPlatform.cpp | 2 +- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/Source/Engine/GraphicsDevice/Vulkan/GPUDeviceVulkan.cpp b/Source/Engine/GraphicsDevice/Vulkan/GPUDeviceVulkan.cpp index 9b6d4ba2c..5e11e1a86 100644 --- a/Source/Engine/GraphicsDevice/Vulkan/GPUDeviceVulkan.cpp +++ b/Source/Engine/GraphicsDevice/Vulkan/GPUDeviceVulkan.cpp @@ -1222,11 +1222,11 @@ GPUDevice* GPUDeviceVulkan::Create() return nullptr; } uint32 vendorId = 0; - if (CommandLine::Options.NVIDIA) + if (CommandLine::Options.NVIDIA.IsTrue()) vendorId = GPU_VENDOR_ID_NVIDIA; - else if (CommandLine::Options.AMD) + else if (CommandLine::Options.AMD.IsTrue()) vendorId = GPU_VENDOR_ID_AMD; - else if (CommandLine::Options.Intel) + else if (CommandLine::Options.Intel.IsTrue()) vendorId = GPU_VENDOR_ID_INTEL; if (vendorId != 0) { diff --git a/Source/Engine/Platform/Linux/LinuxPlatform.cpp b/Source/Engine/Platform/Linux/LinuxPlatform.cpp index 2c951effa..681b60c37 100644 --- a/Source/Engine/Platform/Linux/LinuxPlatform.cpp +++ b/Source/Engine/Platform/Linux/LinuxPlatform.cpp @@ -653,7 +653,7 @@ static int X11_MessageBoxLoop(MessageBoxData* data) DialogResult MessageBox::Show(Window* parent, const StringView& text, const StringView& caption, MessageBoxButtons buttons, MessageBoxIcon icon) { - if (CommandLine::Options.Headless) + if (CommandLine::Options.Headless.IsTrue()) return DialogResult::None; // Setup for simple popup @@ -1369,7 +1369,7 @@ public: DragDropEffect LinuxWindow::DoDragDrop(const StringView& data) { - if (CommandLine::Options.Headless) + if (CommandLine::Options.Headless.IsTrue()) return DragDropEffect::None; auto cursorWrong = X11::XCreateFontCursor(xDisplay, 54); auto cursorTransient = X11::XCreateFontCursor(xDisplay, 24); @@ -1673,7 +1673,7 @@ void LinuxClipboard::Clear() void LinuxClipboard::SetText(const StringView& text) { - if (CommandLine::Options.Headless) + if (CommandLine::Options.Headless.IsTrue()) return; auto mainWindow = (LinuxWindow*)Engine::MainWindow; if (!mainWindow) @@ -1695,7 +1695,7 @@ void LinuxClipboard::SetFiles(const Array& files) String LinuxClipboard::GetText() { - if (CommandLine::Options.Headless) + if (CommandLine::Options.Headless.IsTrue()) return String::Empty; String result; auto mainWindow = (LinuxWindow*)Engine::MainWindow; @@ -2118,7 +2118,7 @@ bool LinuxPlatform::Init() Platform::MemoryClear(CursorsImg, sizeof(CursorsImg)); // Skip setup if running in headless mode (X11 might not be available on servers) - if (CommandLine::Options.Headless) + if (CommandLine::Options.Headless.IsTrue()) return false; X11::XInitThreads(); diff --git a/Source/Engine/Platform/Mac/MacPlatform.cpp b/Source/Engine/Platform/Mac/MacPlatform.cpp index 2f054f7a6..84279e194 100644 --- a/Source/Engine/Platform/Mac/MacPlatform.cpp +++ b/Source/Engine/Platform/Mac/MacPlatform.cpp @@ -54,7 +54,7 @@ String ComputerName; DialogResult MessageBox::Show(Window* parent, const StringView& text, const StringView& caption, MessageBoxButtons buttons, MessageBoxIcon icon) { - if (CommandLine::Options.Headless) + if (CommandLine::Options.Headless.IsTrue()) return DialogResult::None; NSAlert* alert = [[NSAlert alloc] init]; ASSERT(alert); From 077ececcf880409ccbf620539eb142aebf45e574 Mon Sep 17 00:00:00 2001 From: Mateusz Karbowiak <69864511+mtszkarbowiak@users.noreply.github.com> Date: Sun, 6 Oct 2024 13:20:00 +0200 Subject: [PATCH 08/14] `Nullable` match --- Source/Engine/Core/Types/Nullable.h | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/Source/Engine/Core/Types/Nullable.h b/Source/Engine/Core/Types/Nullable.h index 9d8f3f65b..e1aae2ea3 100644 --- a/Source/Engine/Core/Types/Nullable.h +++ b/Source/Engine/Core/Types/Nullable.h @@ -331,6 +331,26 @@ public: { return _hasValue; } + + + /// + /// Matches the wrapped value with a handler for the value or a handler for the null value. + /// + /// Value visitor handling valid nullable value. + /// Null visitor handling invalid nullable value. + /// Result of the call of one of handlers. Handlers must share the same result type. + template + FORCE_INLINE auto Match(ValueVisitor valueHandler, NullVisitor nullHandler) const + { + if (_hasValue) + { + return valueHandler(_value); + } + else + { + return nullHandler(); + } + } }; /// From cda74f5cc472cccdf8e16bce5497ced652211b06 Mon Sep 17 00:00:00 2001 From: Mateusz Karbowiak <69864511+mtszkarbowiak@users.noreply.github.com> Date: Sun, 6 Oct 2024 13:50:03 +0200 Subject: [PATCH 09/14] `Nullable` tests --- Source/Engine/Tests/TestNullable.cpp | 154 +++++++++++++++++++++++++++ 1 file changed, 154 insertions(+) create mode 100644 Source/Engine/Tests/TestNullable.cpp diff --git a/Source/Engine/Tests/TestNullable.cpp b/Source/Engine/Tests/TestNullable.cpp new file mode 100644 index 000000000..945867a04 --- /dev/null +++ b/Source/Engine/Tests/TestNullable.cpp @@ -0,0 +1,154 @@ +// Copyright (c) 2012-2024 Wojciech Figat. All rights reserved. + +#include "Engine/Core/Types/Nullable.h" +#include + +TEST_CASE("Nullable") +{ + SECTION("Trivial Type") + { + Nullable a; + + REQUIRE(a.HasValue() == false); + REQUIRE(a.GetValueOr(2) == 2); + + a = 1; + + REQUIRE(a.HasValue() == true); + REQUIRE(a.GetValue() == 1); + REQUIRE(a.GetValueOr(2) == 1); + + a.Reset(); + + REQUIRE(a.HasValue() == false); + } + + SECTION("Move-Only Type") + { + struct MoveOnly + { + MoveOnly() = default; + ~MoveOnly() = default; + + MoveOnly(const MoveOnly&) = delete; + MoveOnly(MoveOnly&&) = default; + + // MoveOnly& operator=(const MoveOnly&) = delete; + // MoveOnly& operator=(MoveOnly&&) = default; + }; + + Nullable a; + + REQUIRE(a.HasValue() == false); + + a = MoveOnly(); + + REQUIRE(a.HasValue() == true); + } + + SECTION("Bool Type") + { + + Nullable a; + + REQUIRE(a.HasValue() == false); + REQUIRE(a.GetValueOr(true) == true); + REQUIRE(a.IsTrue() == false); + REQUIRE(a.IsFalse() == false); + + a = false; + + REQUIRE(a.HasValue() == true); + REQUIRE(a.GetValue() == false); + REQUIRE(a.GetValueOr(true) == false); + + REQUIRE(a.IsTrue() == false); + REQUIRE(a.IsFalse() == true); + + a = true; + + REQUIRE(a.IsTrue() == true); + REQUIRE(a.IsFalse() == false); + + a.Reset(); + + REQUIRE(a.HasValue() == false); + } + + SECTION("Lifetime (No Construction)") + { + struct DoNotConstruct + { + DoNotConstruct() { FAIL("DoNotConstruct must not be constructed."); } + }; + + Nullable a; + + a.Reset(); + } + + SECTION("Lifetime") + { + int constructed = 0, destructed = 0; + + struct Lifetime + { + int& constructed; + int& destructed; + + Lifetime(int& constructed, int& destructed) + : constructed(constructed) + , destructed(destructed) + { + constructed++; + } + + Lifetime(const Lifetime& other) + : constructed(other.constructed) + , destructed(other.destructed) + { + constructed++; + } + + ~Lifetime() + { + destructed++; + } + }; + + { + Nullable a = Lifetime(constructed, destructed); + + REQUIRE(constructed == 1); + REQUIRE(destructed == 0); + + a.Reset(); + + REQUIRE(constructed == 1); + REQUIRE(destructed == 1); + } + + { + Nullable a = Lifetime(constructed, destructed); + } + + REQUIRE(constructed == 2); + REQUIRE(destructed == 2); + } + + SECTION("Matching") + { + Nullable a; + Nullable b = 2; + + a.Match( + [](int value) { FAIL("Null nullable must not match value handler."); }, + []() {} + ); + + b.Match( + [](int value) {}, + []() { FAIL("Nullable with valid value must not match null handler."); } + ); + } +}; From 44dad402f610f6dd01fe0ce32c65b51523625ef6 Mon Sep 17 00:00:00 2001 From: Mateusz Karbowiak <69864511+mtszkarbowiak@users.noreply.github.com> Date: Mon, 7 Oct 2024 02:41:07 +0200 Subject: [PATCH 10/14] `Nullable` dependency headers fix --- Source/Engine/Core/Types/Nullable.h | 1 + 1 file changed, 1 insertion(+) diff --git a/Source/Engine/Core/Types/Nullable.h b/Source/Engine/Core/Types/Nullable.h index e1aae2ea3..6ca735d08 100644 --- a/Source/Engine/Core/Types/Nullable.h +++ b/Source/Engine/Core/Types/Nullable.h @@ -2,6 +2,7 @@ #pragma once +#include "Engine/Core/Templates.h" #include "Engine/Platform/Platform.h" /// From 541ca67a061916f777c784d6b2a5cb02d4234bf4 Mon Sep 17 00:00:00 2001 From: Mateusz Karbowiak <69864511+mtszkarbowiak@users.noreply.github.com> Date: Mon, 7 Oct 2024 03:11:50 +0200 Subject: [PATCH 11/14] `Nullable` sfinae fix --- Source/Engine/Core/Types/Nullable.h | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/Source/Engine/Core/Types/Nullable.h b/Source/Engine/Core/Types/Nullable.h index 6ca735d08..985e66090 100644 --- a/Source/Engine/Core/Types/Nullable.h +++ b/Source/Engine/Core/Types/Nullable.h @@ -35,11 +35,6 @@ private: } } - /// - /// true if the wrapped type is copy constructible. - /// - constexpr static bool IsCopyConstructible = TIsCopyConstructible::Value; - public: /// /// Initializes by setting the wrapped value to null. @@ -60,7 +55,7 @@ public: /// Initializes by copying the wrapped value. /// /// The initial wrapped value to be copied. - template::Type> + template::Value>::Type> Nullable(const T& value) : _value(value) , _hasValue(true) @@ -81,7 +76,7 @@ public: /// Initializes by copying another . /// /// The wrapped value to be copied. - template::Type> + template::Value>::Type> Nullable(const Nullable& other) : _value(other._value) , _hasValue(other._hasValue) @@ -106,7 +101,7 @@ public: /// /// Reassigns the wrapped value by copying. /// - template::Type> + template::Value>::Type> auto operator=(const T& value) -> Nullable& { KillOld(); @@ -133,7 +128,7 @@ public: /// /// Reassigns the wrapped value by copying another . /// - template::Type> + template::Value>::Type> auto operator=(const Nullable& other) -> Nullable& { KillOld(); @@ -222,7 +217,7 @@ public: /// Sets the wrapped value by copying. /// /// The value to be copied. - template::Type> + template::Value>::Type> FORCE_INLINE void SetValue(const T& value) { if (_hasValue) @@ -250,7 +245,7 @@ public: /// If the wrapped value is not valid, sets it by copying. Otherwise, does nothing. /// /// True if the wrapped value was changed, otherwise false. - template::Type> + template::Value>::Type> FORCE_INLINE bool TrySet(const T& value) { if (_hasValue) From f56207f1a40846749ea9122111d81b1dbf8a611b Mon Sep 17 00:00:00 2001 From: Mateusz Karbowiak <69864511+mtszkarbowiak@users.noreply.github.com> Date: Mon, 7 Oct 2024 12:17:23 +0200 Subject: [PATCH 12/14] `Nullable.Reset` fix, killing inlining --- Source/Engine/Core/Types/Nullable.h | 55 ++++++++++++++++++----------- 1 file changed, 35 insertions(+), 20 deletions(-) diff --git a/Source/Engine/Core/Types/Nullable.h b/Source/Engine/Core/Types/Nullable.h index 985e66090..43fe5342a 100644 --- a/Source/Engine/Core/Types/Nullable.h +++ b/Source/Engine/Core/Types/Nullable.h @@ -24,17 +24,6 @@ private: }; bool _hasValue; - /// - /// Ends the lifetime of the wrapped value by calling its destructor, if the lifetime has not ended yet. Otherwise, does nothing. - /// - FORCE_INLINE void KillOld() - { - if (_hasValue) - { - _value.~T(); - } - } - public: /// /// Initializes by setting the wrapped value to null. @@ -48,7 +37,10 @@ public: ~Nullable() { - KillOld(); + if (_hasValue) + { + _value.~T(); + } } /// @@ -104,7 +96,10 @@ public: template::Value>::Type> auto operator=(const T& value) -> Nullable& { - KillOld(); + if (_hasValue) + { + _value.~T(); + } new (&_value) T(value); // Placement new (copy constructor) _hasValue = true; @@ -117,7 +112,10 @@ public: /// auto operator=(T&& value) noexcept -> Nullable& { - KillOld(); + if (_hasValue) + { + _value.~T(); + } new (&_value) T(MoveTemp(value)); // Placement new (move constructor) _hasValue = true; @@ -131,12 +129,16 @@ public: template::Value>::Type> auto operator=(const Nullable& other) -> Nullable& { - KillOld(); + if (_hasValue) + { + _value.~T(); + } if (other._hasValue) { new (&_value) T(other._value); // Placement new (copy constructor) } + _hasValue = other._hasValue; // Set the flag AFTER the value is copied. return *this; @@ -152,14 +154,19 @@ public: return *this; } - KillOld(); - if (_hasValue) + { + _value.~T(); + } + + if (other._hasValue) { new (&_value) T(MoveTemp(other._value)); // Placement new (move constructor) - other.Reset(); + other._value.~T(); // Kill the old value in the source object. + other._hasValue = false; } + _hasValue = other._hasValue; // Set the flag AFTER the value is moved. return *this; @@ -235,7 +242,10 @@ public: /// The value to be moved. FORCE_INLINE void SetValue(T&& value) noexcept { - KillOld(); + if (_hasValue) + { + _value.~T(); + } new (&_value) T(MoveTemp(value)); // Placement new (move constructor) _hasValue = true; // Set the flag AFTER the value is moved. @@ -279,8 +289,13 @@ public: /// FORCE_INLINE void Reset() { + if (!_hasValue) + { + return; + } + _hasValue = false; // Reset the flag BEFORE the value is (potentially) destructed. - KillOld(); + _value.~T(); } /// From eda4f433d035917dc1c9e1cf84a5f5537ab8656e Mon Sep 17 00:00:00 2001 From: Mateusz Karbowiak <69864511+mtszkarbowiak@users.noreply.github.com> Date: Mon, 7 Oct 2024 12:24:09 +0200 Subject: [PATCH 13/14] Update TestNullable.cpp --- Source/Engine/Tests/TestNullable.cpp | 155 ++++++++++++++------------- 1 file changed, 82 insertions(+), 73 deletions(-) diff --git a/Source/Engine/Tests/TestNullable.cpp b/Source/Engine/Tests/TestNullable.cpp index 945867a04..d02d98c0d 100644 --- a/Source/Engine/Tests/TestNullable.cpp +++ b/Source/Engine/Tests/TestNullable.cpp @@ -7,7 +7,7 @@ TEST_CASE("Nullable") { SECTION("Trivial Type") { - Nullable a; + Nullable a; REQUIRE(a.HasValue() == false); REQUIRE(a.GetValueOr(2) == 2); @@ -25,130 +25,139 @@ TEST_CASE("Nullable") SECTION("Move-Only Type") { - struct MoveOnly - { - MoveOnly() = default; + struct MoveOnly + { + MoveOnly() = default; ~MoveOnly() = default; - MoveOnly(const MoveOnly&) = delete; - MoveOnly(MoveOnly&&) = default; + MoveOnly(const MoveOnly&) = delete; + MoveOnly(MoveOnly&&) = default; - // MoveOnly& operator=(const MoveOnly&) = delete; - // MoveOnly& operator=(MoveOnly&&) = default; - }; + MoveOnly& operator=(const MoveOnly&) = delete; + MoveOnly& operator=(MoveOnly&&) = default; + }; - Nullable a; + Nullable a; - REQUIRE(a.HasValue() == false); + REQUIRE(a.HasValue() == false); - a = MoveOnly(); + a = MoveOnly(); - REQUIRE(a.HasValue() == true); + REQUIRE(a.HasValue() == true); } SECTION("Bool Type") { - - Nullable a; + Nullable a; - REQUIRE(a.HasValue() == false); - REQUIRE(a.GetValueOr(true) == true); + REQUIRE(a.HasValue() == false); + REQUIRE(a.GetValueOr(true) == true); REQUIRE(a.IsTrue() == false); REQUIRE(a.IsFalse() == false); - a = false; + a = false; - REQUIRE(a.HasValue() == true); - REQUIRE(a.GetValue() == false); - REQUIRE(a.GetValueOr(true) == false); + REQUIRE(a.HasValue() == true); + REQUIRE(a.GetValue() == false); + REQUIRE(a.GetValueOr(true) == false); - REQUIRE(a.IsTrue() == false); - REQUIRE(a.IsFalse() == true); + REQUIRE(a.IsTrue() == false); + REQUIRE(a.IsFalse() == true); a = true; REQUIRE(a.IsTrue() == true); REQUIRE(a.IsFalse() == false); - a.Reset(); + a.Reset(); - REQUIRE(a.HasValue() == false); + REQUIRE(a.HasValue() == false); } SECTION("Lifetime (No Construction)") { - struct DoNotConstruct - { - DoNotConstruct() { FAIL("DoNotConstruct must not be constructed."); } - }; + struct DoNotConstruct + { + DoNotConstruct() { FAIL("DoNotConstruct must not be constructed."); } + }; Nullable a; - a.Reset(); } SECTION("Lifetime") - { - int constructed = 0, destructed = 0; - + { struct Lifetime - { - int& constructed; - int& destructed; + { + int* _constructed; + int* _destructed; - Lifetime(int& constructed, int& destructed) - : constructed(constructed) - , destructed(destructed) - { - constructed++; - } + Lifetime(int* constructed, int* destructed) + : _constructed(constructed) + , _destructed(destructed) + { + ++(*_constructed); + } - Lifetime(const Lifetime& other) - : constructed(other.constructed) - , destructed(other.destructed) - { - constructed++; - } + Lifetime(Lifetime&& other) noexcept + : _constructed(other._constructed) + , _destructed(other._destructed) + { + ++(*_constructed); + } - ~Lifetime() - { - destructed++; - } - }; + Lifetime() = delete; + Lifetime& operator=(const Lifetime&) = delete; + Lifetime& operator=(Lifetime&&) = delete; - { - Nullable a = Lifetime(constructed, destructed); + ~Lifetime() + { + ++(*_destructed); + } + }; - REQUIRE(constructed == 1); - REQUIRE(destructed == 0); + int constructed = 0, destructed = 0; + REQUIRE(constructed == destructed); - a.Reset(); + { - REQUIRE(constructed == 1); - REQUIRE(destructed == 1); - } + Nullable a = Lifetime(&constructed, &destructed); + REQUIRE(a.HasValue()); + REQUIRE(constructed == destructed + 1); - { - Nullable a = Lifetime(constructed, destructed); - } + a.Reset(); + REQUIRE(!a.HasValue()); + REQUIRE(constructed == destructed); + } + REQUIRE(constructed == destructed); - REQUIRE(constructed == 2); - REQUIRE(destructed == 2); - } + { + Nullable b = Lifetime(&constructed, &destructed); + REQUIRE(constructed == destructed + 1); + } + REQUIRE(constructed == destructed); + + { + Nullable c = Lifetime(&constructed, &destructed); + Nullable d = MoveTemp(c); + REQUIRE(constructed == destructed + 1); + } + REQUIRE(constructed == destructed); + } SECTION("Matching") { - Nullable a; - Nullable b = 2; + Nullable a; + Nullable b = 2; a.Match( - [](int value) { FAIL("Null nullable must not match value handler."); }, - []() {} + [](int) { FAIL("Null nullable must not match value handler."); }, + []() {} ); b.Match( - [](int value) {}, + [](int) {}, []() { FAIL("Nullable with valid value must not match null handler."); } - ); + ); } }; From fa9ce1d346e1add2060b237c883d41471e98abcb Mon Sep 17 00:00:00 2001 From: Wojtek Figat Date: Fri, 25 Oct 2024 15:59:38 +0200 Subject: [PATCH 14/14] Code formatting #2969 --- Source/Engine/Core/Types/Nullable.h | 66 +++++------------------------ 1 file changed, 10 insertions(+), 56 deletions(-) diff --git a/Source/Engine/Core/Types/Nullable.h b/Source/Engine/Core/Types/Nullable.h index 43fe5342a..4bebfbf85 100644 --- a/Source/Engine/Core/Types/Nullable.h +++ b/Source/Engine/Core/Types/Nullable.h @@ -19,7 +19,7 @@ private: union { - T _value; + T _value; Dummy _dummy; }; bool _hasValue; @@ -32,15 +32,12 @@ public: : _dummy() , _hasValue(false) { - // Value is not initialized. } ~Nullable() { if (_hasValue) - { _value.~T(); - } } /// @@ -82,9 +79,7 @@ public: Nullable(Nullable&& other) noexcept { if (other._hasValue) - { new (&_value) T(MoveTemp(other._value)); // Placement new (move constructor) - } _hasValue = other._hasValue; other.Reset(); @@ -94,12 +89,10 @@ public: /// Reassigns the wrapped value by copying. /// template::Value>::Type> - auto operator=(const T& value) -> Nullable& + Nullable& operator=(const T& value) { if (_hasValue) - { _value.~T(); - } new (&_value) T(value); // Placement new (copy constructor) _hasValue = true; @@ -110,12 +103,10 @@ public: /// /// Reassigns the wrapped value by moving. /// - auto operator=(T&& value) noexcept -> Nullable& + Nullable& operator=(T&& value) noexcept { if (_hasValue) - { _value.~T(); - } new (&_value) T(MoveTemp(value)); // Placement new (move constructor) _hasValue = true; @@ -127,17 +118,13 @@ public: /// Reassigns the wrapped value by copying another . /// template::Value>::Type> - auto operator=(const Nullable& other) -> Nullable& + Nullable& operator=(const Nullable& other) { if (_hasValue) - { _value.~T(); - } if (other._hasValue) - { new (&_value) T(other._value); // Placement new (copy constructor) - } _hasValue = other._hasValue; // Set the flag AFTER the value is copied. @@ -147,17 +134,13 @@ public: /// /// Reassigns the wrapped value by moving another . /// - auto operator=(Nullable&& other) noexcept -> Nullable& + Nullable& operator=(Nullable&& other) noexcept { if (this == &other) - { return *this; - } if (_hasValue) - { _value.~T(); - } if (other._hasValue) { @@ -228,10 +211,7 @@ public: FORCE_INLINE void SetValue(const T& value) { if (_hasValue) - { _value.~T(); - } - new (&_value) T(value); // Placement new (copy constructor) _hasValue = true; // Set the flag AFTER the value is copied. } @@ -243,10 +223,7 @@ public: FORCE_INLINE void SetValue(T&& value) noexcept { if (_hasValue) - { _value.~T(); - } - new (&_value) T(MoveTemp(value)); // Placement new (move constructor) _hasValue = true; // Set the flag AFTER the value is moved. } @@ -259,10 +236,7 @@ public: FORCE_INLINE bool TrySet(const T& value) { if (_hasValue) - { return false; - } - new (&_value) T(value); // Placement new (copy constructor) _hasValue = true; // Set the flag AFTER the value is copied. return true; @@ -275,10 +249,7 @@ public: FORCE_INLINE bool TrySet(T&& value) noexcept { if (_hasValue) - { return false; - } - new (&_value) T(MoveTemp(value)); // Placement new (move constructor) _hasValue = true; // Set the flag AFTER the value is moved. return true; @@ -290,10 +261,7 @@ public: FORCE_INLINE void Reset() { if (!_hasValue) - { return; - } - _hasValue = false; // Reset the flag BEFORE the value is (potentially) destructed. _value.~T(); } @@ -316,12 +284,7 @@ public: /// true if both values are equal. FORCE_INLINE bool operator==(const Nullable& other) const { - if (other._hasValue != _hasValue) - { - return false; - } - - return _value == other._value; + return other._hasValue == _hasValue && _value == other._value; } /// @@ -343,7 +306,6 @@ public: return _hasValue; } - /// /// Matches the wrapped value with a handler for the value or a handler for the null value. /// @@ -354,13 +316,8 @@ public: FORCE_INLINE auto Match(ValueVisitor valueHandler, NullVisitor nullHandler) const { if (_hasValue) - { return valueHandler(_value); - } - else - { - return nullHandler(); - } + return nullHandler(); } }; @@ -409,11 +366,10 @@ public: _value = value ? Value::True : Value::False; } - /// /// Reassigns the wrapped value by implicitly casting a boolean value. /// - auto operator=(const bool value) noexcept -> Nullable& + Nullable& operator=(const bool value) noexcept { _value = value ? Value::True : Value::False; return *this; @@ -422,13 +378,12 @@ public: /// /// Reassigns the wrapped value by copying another nullable boolean. /// - auto operator=(const Nullable& value) -> Nullable& = default; + Nullable& operator=(const Nullable& value) = default; /// /// Reassigns the wrapped value by moving another nullable boolean. /// - auto operator=(Nullable&& value) -> Nullable& = default; - + Nullable& operator=(Nullable&& value) = default; /// /// Checks if wrapped bool has a valid value. @@ -495,7 +450,6 @@ public: _value = Value::Null; } - /// /// Checks if the current object has a valid value and it's set to true. If the value is false or not valid, the method returns false. ///