From 47a001722d7ada681e333e991373e19d159c380b Mon Sep 17 00:00:00 2001 From: Tony Wasserka Date: Thu, 29 May 2014 22:54:34 +0200 Subject: [PATCH 1/4] BitField: Add an explicit evaluation method. Sometimes it can be beneficial to use this in places where an explicit cast needs to happen otherwise. By using the evaluation method, it's not necessary anymore to explicitly write the underlying type in this case. --- src/common/bit_field.h | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/common/bit_field.h b/src/common/bit_field.h index dfd00d198f..a39bb98867 100644 --- a/src/common/bit_field.h +++ b/src/common/bit_field.h @@ -131,6 +131,11 @@ public: } __forceinline operator T() const + { + return Value(); + } + + __forceinline T Value() const { if (std::numeric_limits::is_signed) { From 15ab5382a553a47f21fe7283010ccdb342c0ead6 Mon Sep 17 00:00:00 2001 From: Tony Wasserka Date: Wed, 16 Jul 2014 09:07:57 +0200 Subject: [PATCH 2/4] BitField: Delete copy assignment to prevent obscure bugs. Cf. https://github.com/dolphin-emu/dolphin/pull/483 --- src/common/bit_field.h | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/common/bit_field.h b/src/common/bit_field.h index a39bb98867..c28e722c80 100644 --- a/src/common/bit_field.h +++ b/src/common/bit_field.h @@ -124,6 +124,22 @@ public: // so that we can use this within unions BitField() = default; +#ifndef _WIN32 + // We explicitly delete the copy assigment operator here, because the + // default copy assignment would copy the full storage value, rather than + // just the bits relevant to this particular bit field. + // Ideally, we would just implement the copy assignment to copy only the + // relevant bits, but this requires compiler support for unrestricted + // unions. + // MSVC 2013 has no support for this, hence we disable this code on + // Windows (so that the default copy assignment operator will be used). + // For any C++11 conformant compiler we delete the operator to make sure + // we never use this inappropriate operator to begin with. + // TODO: Implement this operator properly once all target compilers + // support unrestricted unions. + BitField& operator=(const BitField&) = delete; +#endif + __forceinline BitField& operator=(T val) { storage = (storage & ~GetMask()) | ((val << position) & GetMask()); From cd1d5786d998efdf18e9938db957f82331bab579 Mon Sep 17 00:00:00 2001 From: Tony Wasserka Date: Sat, 31 May 2014 19:04:35 +0200 Subject: [PATCH 3/4] BitField: Add a static_assert. Being able to store BitField within unions requires BitField to be of standard layout, which in turn is only given if the underlying type is also has standard layout. --- src/common/bit_field.h | 1 + 1 file changed, 1 insertion(+) diff --git a/src/common/bit_field.h b/src/common/bit_field.h index c28e722c80..52f0d8be6b 100644 --- a/src/common/bit_field.h +++ b/src/common/bit_field.h @@ -189,5 +189,6 @@ private: static_assert(position < 8 * sizeof(T), "Invalid position"); static_assert(bits <= 8 * sizeof(T), "Invalid number of bits"); static_assert(bits > 0, "Invalid number of bits"); + static_assert(std::is_standard_layout::value, "Invalid base type"); }; #pragma pack() From 0da8e2eacc95164e3c1d32c8177cc044ed0f1930 Mon Sep 17 00:00:00 2001 From: Tony Wasserka Date: Wed, 16 Jul 2014 09:39:52 +0200 Subject: [PATCH 4/4] BitField: Cast enum values to proper integer type. --- src/common/bit_field.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/common/bit_field.h b/src/common/bit_field.h index 52f0d8be6b..b6f0179c65 100644 --- a/src/common/bit_field.h +++ b/src/common/bit_field.h @@ -142,7 +142,7 @@ public: __forceinline BitField& operator=(T val) { - storage = (storage & ~GetMask()) | ((val << position) & GetMask()); + storage = (storage & ~GetMask()) | (((StorageType)val << position) & GetMask()); return *this; }