From e1c57265d95c7699d4c03d70482fce5794fda0f6 Mon Sep 17 00:00:00 2001 From: Chester Liu Date: Fri, 9 Jul 2021 13:07:11 +0800 Subject: [PATCH 1/3] Use memcmp for TextAttribute & TextColor comparison --- src/buffer/out/TextAttribute.cpp | 2 ++ src/buffer/out/TextAttribute.hpp | 31 ++++++++++--------------------- src/buffer/out/TextColor.cpp | 6 ++++++ src/buffer/out/TextColor.h | 26 +++++++++----------------- src/renderer/base/renderer.cpp | 2 +- 5 files changed, 28 insertions(+), 39 deletions(-) diff --git a/src/buffer/out/TextAttribute.cpp b/src/buffer/out/TextAttribute.cpp index 2791bc352e1..db8f2700ec4 100644 --- a/src/buffer/out/TextAttribute.cpp +++ b/src/buffer/out/TextAttribute.cpp @@ -11,6 +11,8 @@ static_assert(sizeof(TextAttribute) == 14); static_assert(alignof(TextAttribute) == 2); // Ensure that we can memcpy() and memmove() the struct for performance. static_assert(std::is_trivially_copyable_v); +// Assert that the use of memcmp() for comparisons is safe. +static_assert(std::has_unique_object_representations_v); BYTE TextAttribute::s_legacyDefaultForeground = 7; BYTE TextAttribute::s_legacyDefaultBackground = 0; diff --git a/src/buffer/out/TextAttribute.hpp b/src/buffer/out/TextAttribute.hpp index eedb6cdd764..1032bd74f5b 100644 --- a/src/buffer/out/TextAttribute.hpp +++ b/src/buffer/out/TextAttribute.hpp @@ -82,12 +82,15 @@ class TextAttribute final void Invert() noexcept; - friend constexpr bool operator==(const TextAttribute& a, const TextAttribute& b) noexcept; - friend constexpr bool operator!=(const TextAttribute& a, const TextAttribute& b) noexcept; - friend constexpr bool operator==(const TextAttribute& attr, const WORD& legacyAttr) noexcept; - friend constexpr bool operator!=(const TextAttribute& attr, const WORD& legacyAttr) noexcept; - friend constexpr bool operator==(const WORD& legacyAttr, const TextAttribute& attr) noexcept; - friend constexpr bool operator!=(const WORD& legacyAttr, const TextAttribute& attr) noexcept; + inline bool operator==(const TextAttribute& other) const noexcept + { + return memcmp(this, &other, sizeof(TextAttribute)) == 0; + } + + inline bool operator!=(const TextAttribute& other) const noexcept + { + return memcmp(this, &other, sizeof(TextAttribute)) != 0; + } bool IsLegacy() const noexcept; bool IsBold() const noexcept; @@ -178,7 +181,7 @@ class TextAttribute final uint16_t _hyperlinkId; // sizeof: 2, alignof: 2 TextColor _foreground; // sizeof: 4, alignof: 1 TextColor _background; // sizeof: 4, alignof: 1 - ExtendedAttributes _extendedAttrs; // sizeof: 1, alignof: 1 + ExtendedAttributes _extendedAttrs; // sizeof: 2, alignof: 2 #ifdef UNIT_TESTING friend class TextBufferTests; @@ -195,20 +198,6 @@ enum class TextAttributeBehavior StoredOnly, // only use the contained text attribute and skip the insertion of anything else }; -constexpr bool operator==(const TextAttribute& a, const TextAttribute& b) noexcept -{ - return a._wAttrLegacy == b._wAttrLegacy && - a._foreground == b._foreground && - a._background == b._background && - a._extendedAttrs == b._extendedAttrs && - a._hyperlinkId == b._hyperlinkId; -} - -constexpr bool operator!=(const TextAttribute& a, const TextAttribute& b) noexcept -{ - return !(a == b); -} - #ifdef UNIT_TESTING #define LOG_ATTR(attr) (Log::Comment(NoThrowString().Format( \ diff --git a/src/buffer/out/TextColor.cpp b/src/buffer/out/TextColor.cpp index 20405022984..512572503f5 100644 --- a/src/buffer/out/TextColor.cpp +++ b/src/buffer/out/TextColor.cpp @@ -50,6 +50,12 @@ constexpr std::array Index256ToIndex16 = { // clang-format on +// We should only need 4B for TextColor. Any more than that is just waste. +static_assert(sizeof(TextColor) <= 4 * sizeof(BYTE), "We should only need 4B for an entire TextColor. Any more than that is just waste"); + +// Assert that the use of memcmp() for comparisons is safe. +static_assert(std::has_unique_object_representations_v); + bool TextColor::CanBeBrightened() const noexcept { return IsIndex16() || IsDefault(); diff --git a/src/buffer/out/TextColor.h b/src/buffer/out/TextColor.h index e8dcfcd8216..1f63ac65080 100644 --- a/src/buffer/out/TextColor.h +++ b/src/buffer/out/TextColor.h @@ -72,8 +72,15 @@ struct TextColor { } - friend constexpr bool operator==(const TextColor& a, const TextColor& b) noexcept; - friend constexpr bool operator!=(const TextColor& a, const TextColor& b) noexcept; + bool operator==(const TextColor& other) const noexcept + { + return memcmp(this, &other, sizeof(TextColor)) == 0; + } + + bool operator!=(const TextColor& other) const noexcept + { + return memcmp(this, &other, sizeof(TextColor)) != 0; + } bool CanBeBrightened() const noexcept; bool IsLegacy() const noexcept; @@ -115,19 +122,6 @@ struct TextColor #endif }; -bool constexpr operator==(const TextColor& a, const TextColor& b) noexcept -{ - return a._meta == b._meta && - a._red == b._red && - a._green == b._green && - a._blue == b._blue; -} - -bool constexpr operator!=(const TextColor& a, const TextColor& b) noexcept -{ - return !(a == b); -} - #ifdef UNIT_TESTING namespace WEX @@ -157,5 +151,3 @@ namespace WEX } } #endif - -static_assert(sizeof(TextColor) <= 4 * sizeof(BYTE), "We should only need 4B for an entire TextColor. Any more than that is just waste"); diff --git a/src/renderer/base/renderer.cpp b/src/renderer/base/renderer.cpp index b53bc6eadf6..b7b2435f633 100644 --- a/src/renderer/base/renderer.cpp +++ b/src/renderer/base/renderer.cpp @@ -786,7 +786,7 @@ void Renderer::_PaintBufferOutputHelper(_In_ IRenderEngine* const pEngine, { COORD thisPoint{ screenPoint.X + gsl::narrow(cols), screenPoint.Y }; const auto thisPointPatterns = _pData->GetPatternId(thisPoint); - if (color != it->TextAttr() || patternIds != thisPointPatterns) + if (color != it->TextAttr() || (thisPointPatterns.size() > 0 && patternIds != thisPointPatterns)) { auto newAttr{ it->TextAttr() }; // foreground doesn't matter for runs of spaces (!) From ae4382e43d1368f0a26e0c3e8e1816c5e8288630 Mon Sep 17 00:00:00 2001 From: Chester Liu Date: Fri, 9 Jul 2021 13:09:06 +0800 Subject: [PATCH 2/3] Missed one --- src/inc/conattrs.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/inc/conattrs.hpp b/src/inc/conattrs.hpp index cb911006435..364fef34aa0 100644 --- a/src/inc/conattrs.hpp +++ b/src/inc/conattrs.hpp @@ -8,7 +8,7 @@ Licensed under the MIT license. #define BG_ATTRS (BACKGROUND_BLUE | BACKGROUND_GREEN | BACKGROUND_RED | BACKGROUND_INTENSITY) #define META_ATTRS (COMMON_LVB_LEADING_BYTE | COMMON_LVB_TRAILING_BYTE | COMMON_LVB_GRID_HORIZONTAL | COMMON_LVB_GRID_LVERTICAL | COMMON_LVB_GRID_RVERTICAL | COMMON_LVB_REVERSE_VIDEO | COMMON_LVB_UNDERSCORE) -enum class ExtendedAttributes : BYTE +enum class ExtendedAttributes : uint16_t { Normal = 0x00, Bold = 0x01, From e84eeae396529e53d7acbc7148d77c2472907fa1 Mon Sep 17 00:00:00 2001 From: Chester Liu Date: Fri, 9 Jul 2021 15:01:55 +0800 Subject: [PATCH 3/3] Nope, doesn't work --- src/buffer/out/TextColor.cpp | 5 +++++ src/renderer/base/renderer.cpp | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/src/buffer/out/TextColor.cpp b/src/buffer/out/TextColor.cpp index 512572503f5..0e5d8d92d9f 100644 --- a/src/buffer/out/TextColor.cpp +++ b/src/buffer/out/TextColor.cpp @@ -112,6 +112,8 @@ void TextColor::SetIndex(const BYTE index, const bool isIndex256) noexcept { _meta = isIndex256 ? ColorType::IsIndex256 : ColorType::IsIndex16; _index = index; + _green = 0; + _blue = 0; } // Method Description: @@ -124,6 +126,9 @@ void TextColor::SetIndex(const BYTE index, const bool isIndex256) noexcept void TextColor::SetDefault() noexcept { _meta = ColorType::IsDefault; + _red = 0; + _green = 0; + _blue = 0; } // Method Description: diff --git a/src/renderer/base/renderer.cpp b/src/renderer/base/renderer.cpp index b7b2435f633..b53bc6eadf6 100644 --- a/src/renderer/base/renderer.cpp +++ b/src/renderer/base/renderer.cpp @@ -786,7 +786,7 @@ void Renderer::_PaintBufferOutputHelper(_In_ IRenderEngine* const pEngine, { COORD thisPoint{ screenPoint.X + gsl::narrow(cols), screenPoint.Y }; const auto thisPointPatterns = _pData->GetPatternId(thisPoint); - if (color != it->TextAttr() || (thisPointPatterns.size() > 0 && patternIds != thisPointPatterns)) + if (color != it->TextAttr() || patternIds != thisPointPatterns) { auto newAttr{ it->TextAttr() }; // foreground doesn't matter for runs of spaces (!)