From 7a895e8e11a5e91ae97f11782500c1d49910237a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Sat, 26 Apr 2025 14:45:34 +0200 Subject: [PATCH] Math: reimplement color literals using variadic templates. This means I don't get the parsed number as an integer but have to parse it myself from individual chars, on the other hand it allows me to check that it's indeed hexadecimal and has correct length, because writing a 24-bit color literal with _rgbaf, or forgetting one digit, etc. was a common source of accidental bugs. The new code is larger and more complex, so I verified that it doesn't have too large effect on compile times. On GCC the MathColorTest built in ~1.20 seconds before and ~1.25 after, on Clang it was 1.24 vs 1.26. So there's a minor increase, but it's small enough to warrant the increased robustness. In total, the whole codebase with tests builds in ~120 seconds both before and after, so I suppose unless the codebase consists of just color literals alone (which could be the case for e.g. some stylesheets), it's completely fine. And it found bugs in some test code already. Fix for those is in next commit. --- doc/changelog.dox | 3 ++ src/Magnum/Math/Color.h | 95 ++++++++++++++++++++++++++++++++--------- 2 files changed, 77 insertions(+), 21 deletions(-) diff --git a/doc/changelog.dox b/doc/changelog.dox index abd11a7bb..676329c5f 100644 --- a/doc/changelog.dox +++ b/doc/changelog.dox @@ -768,6 +768,9 @@ See also: overloads taking a scalar - Added a @ref Math::join(const Range&, const Vector&) overload for joining a range and a point +- All color literals are now implemented in a way that checks for them to be + hexadecimal and of correct length (6 digits for @ref Math::Color3 and + 8 digits for @ref Math::Color4) at compile time to prevent common errors @subsubsection changelog-latest-changes-meshtools MeshTools library diff --git a/src/Magnum/Math/Color.h b/src/Magnum/Math/Color.h index f6a0edea7..1a14f78b8 100644 --- a/src/Magnum/Math/Color.h +++ b/src/Magnum/Math/Color.h @@ -1404,6 +1404,64 @@ extern template MAGNUM_EXPORT Debug& operator<<(Debug&, const ColorHsv&); #endif #endif +namespace Implementation { + +/* Color literal parsing code. Not using the operator""(unsigned long long) + because this allows for ensuring that it's always hexadecimal and has + exactly 6 or 8 digits, avoiding common bugs. Has to be outside of the + ColorLiterals namespace because otherwise it leads to ambiguous namespace + reference (ugh?). + + The default template arguments and the final variadic argument are here to + avoid cascaded errors beyond the static_assert when the literal isn't used + with exactly 6+2 chars */ +template constexpr Color color3Literal() { + /* The compiler only allows numeric literals, so we only need to verify + that it starts with 0x or 0X (and thus isn't octal, binary or decimal) + and that there's exactly the amount of digits we want after. There are + however also hex float literals such as 0xc.f3P1. The period is optional + and the P can be on any position except the first or last, plus it can + be both lowercase and uppercase. + + Finally, the most common error is too many or too little characters, so + that's the first check here, and it's written like this instead of + `size == 2 + 6` so when the compiler prints the error details, it shows + up as e.g. "note: the comparison reduces to ‘(7 == 6)’". */ + static_assert(size - 2 == 6 && zero == '0' && (x|0x20) == 'x' && (r2|0x20) != 'p' && (g1|0x20) != 'p' && (g2|0x20) != 'p' && (b1|0x20) != 'p', + "expected a hexadecimal 24-bit color literal"); + typedef typename Color::Type T; + return { + /* The unsigned cast needs to be there to avoid warnings with negative + shift if any char is a period (with a float or hex float literal). + The |0x20 turns uppercase into lowercase, the 'W' is 'a' - 10, just + to not repeat it everywhere. */ + T((unsigned(r1 <= '9' ? r1 - '0' : (r1|0x20) - 'W') << 4)| + unsigned(r2 <= '9' ? r2 - '0' : (r2|0x20) - 'W'))/T(divisor), + T((unsigned(g1 <= '9' ? g1 - '0' : (g1|0x20) - 'W') << 4)| + unsigned(g2 <= '9' ? g2 - '0' : (g2|0x20) - 'W'))/T(divisor), + T((unsigned(b1 <= '9' ? b1 - '0' : (b1|0x20) - 'W') << 4)| + unsigned(b2 <= '9' ? b2 - '0' : (b2|0x20) - 'W'))/T(divisor) + }; +} +/* Same, just for RGBA, so 8+2 chars */ +template constexpr Color color4Literal() { + static_assert(size - 2 == 8 && zero == '0' && (x|0x20) == 'x' && (r2|0x20) != 'p' && (g1|0x20) != 'p' && (g2|0x20) != 'p' && (b1|0x20) != 'p' && (b2|0x20) != 'p' && (a1|0x20) != 'p', + "expected a hexadecimal 32-bit color literal"); + typedef typename Color::Type T; + return { + T((unsigned(r1 <= '9' ? r1 - '0' : (r1|0x20) - 'W') << 4)| + unsigned(r2 <= '9' ? r2 - '0' : (r2|0x20) - 'W'))/T(divisor), + T((unsigned(g1 <= '9' ? g1 - '0' : (g1|0x20) - 'W') << 4)| + unsigned(g2 <= '9' ? g2 - '0' : (g2|0x20) - 'W'))/T(divisor), + T((unsigned(b1 <= '9' ? b1 - '0' : (b1|0x20) - 'W') << 4)| + unsigned(b2 <= '9' ? b2 - '0' : (b2|0x20) - 'W'))/T(divisor), + T((unsigned(a1 <= '9' ? a1 - '0' : (a1|0x20) - 'W') << 4)| + unsigned(a2 <= '9' ? a2 - '0' : (a2|0x20) - 'W'))/T(divisor) + }; +} + +} + /* Unlike STL, where there's e.g. std::literals::string_literals with both being inline, here's just the second inline because making both would cause the literals to be implicitly available to all code in Math. Which isn't @@ -1448,8 +1506,8 @@ Unpacks the literal into three 8-bit values. Example usage: @see @link operator""_rgba() @endlink, @link operator""_rgbf() @endlink @m_keywords{_rgb rgb} */ -constexpr Color3 operator"" _rgb(unsigned long long value) { - return {UnsignedByte(value >> 16), UnsignedByte(value >> 8), UnsignedByte(value)}; +template constexpr Color3 operator"" _rgb() { + return Implementation::color3Literal, 1, sizeof...(chars), chars...>(); } /** @relatesalso Magnum::Math::Color3 @@ -1474,8 +1532,8 @@ RGB. Use this literal to document that given value is in sRGB. Example usage: */ /* Output is a Vector3 to hint that it doesn't have any (additive, multiplicative) semantics of a linear RGB color */ -constexpr Vector3 operator"" _srgb(unsigned long long value) { - return {UnsignedByte(value >> 16), UnsignedByte(value >> 8), UnsignedByte(value)}; +template constexpr Vector3 operator"" _srgb() { + return Implementation::color3Literal, 1, sizeof...(chars), chars...>(); } /** @relatesalso Magnum::Math::Color4 @@ -1493,8 +1551,8 @@ Unpacks the literal into four 8-bit values. Example usage: @see @link operator""_rgb() @endlink, @link operator""_rgbaf() @endlink @m_keywords{_rgba rgba} */ -constexpr Color4 operator"" _rgba(unsigned long long value) { - return {UnsignedByte(value >> 24), UnsignedByte(value >> 16), UnsignedByte(value >> 8), UnsignedByte(value)}; +template constexpr Color4 operator"" _rgba() { + return Implementation::color4Literal, 1, sizeof...(chars), chars...>(); } /** @relatesalso Magnum::Math::Color4 @@ -1520,8 +1578,8 @@ usage: */ /* Output is a Vector3 to hint that it doesn't have any (additive, multiplicative) semantics of a linear RGB color */ -constexpr Vector4 operator"" _srgba(unsigned long long value) { - return {UnsignedByte(value >> 24), UnsignedByte(value >> 16), UnsignedByte(value >> 8), UnsignedByte(value)}; +template constexpr Vector4 operator"" _srgba() { + return Implementation::color4Literal, 1, sizeof...(chars), chars...>(); } /** @relatesalso Magnum::Math::Color3 @@ -1540,10 +1598,8 @@ Example usage: @see @link operator""_rgbaf() @endlink, @link operator""_rgb() @endlink @m_keywords{_rgbf rgbf} */ -constexpr Color3 operator"" _rgbf(unsigned long long value) { - return {((value >> 16) & 0xff)/255.0f, - ((value >> 8) & 0xff)/255.0f, - ((value >> 0) & 0xff)/255.0f}; +template constexpr Color3 operator"" _rgbf() { + return Implementation::color3Literal, 255, sizeof...(chars), chars...>(); } /** @relatesalso Magnum::Math::Color3 @@ -1557,8 +1613,8 @@ Calls @ref Color3::fromSrgbInt() on the literal value. Example usage: @link operator""_rgbf() @endlink @m_keywords{_srgbf srgbf} */ -inline Color3 operator"" _srgbf(unsigned long long value) { - return Color3::fromSrgbInt(UnsignedInt(value)); +template inline Color3 operator"" _srgbf() { + return Color3::fromSrgb(Implementation::color3Literal, 1, sizeof...(chars), chars...>()); } /** @relatesalso Magnum::Math::Color4 @@ -1577,11 +1633,8 @@ Example usage: @see @link operator""_rgbf() @endlink, @link operator""_rgba() @endlink @m_keywords{_rgbaf rgbaf} */ -constexpr Color4 operator"" _rgbaf(unsigned long long value) { - return {((value >> 24) & 0xff)/255.0f, - ((value >> 16) & 0xff)/255.0f, - ((value >> 8) & 0xff)/255.0f, - ((value >> 0) & 0xff)/255.0f}; +template constexpr Color4 operator"" _rgbaf() { + return Implementation::color4Literal, 255, sizeof...(chars), chars...>(); } /** @relatesalso Magnum::Math::Color4 @@ -1595,8 +1648,8 @@ Calls @ref Color4::fromSrgbAlphaInt() on the literal value. Example usage: @link operator""_rgbaf() @endlink @m_keywords{_srgbaf srgbaf} */ -inline Color4 operator"" _srgbaf(unsigned long long value) { - return Color4::fromSrgbAlphaInt(UnsignedInt(value)); +template inline Color4 operator"" _srgbaf() { + return Color4::fromSrgbAlpha(Implementation::color4Literal, 1, sizeof...(chars), chars...>()); } #if defined(CORRADE_TARGET_CLANG) && __clang_major__ >= 17 #pragma clang diagnostic pop