Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Don't use memcmp() to compare TextAttributes, instead just reorder the data member comparisons #12866

Closed
wants to merge 3 commits into from
Closed

Don't use memcmp() to compare TextAttributes, instead just reorder the data member comparisons #12866

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4ca3340
Change operator== for TextAttribute to compare the data members in ph…
EmJayGee Apr 8, 2022
b10b903
Merge branch 'textattributeopequal' of https://github.com/EmJayGee/te…
EmJayGee Apr 8, 2022
459a1f5
Fix spelling error in comments
EmJayGee Apr 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions src/buffer/out/TextAttribute.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,7 @@ class TextAttribute final
TextColor _foreground; // sizeof: 4, alignof: 1
TextColor _background; // sizeof: 4, alignof: 1
ExtendedAttributes _extendedAttrs; // sizeof: 1, alignof: 1
// Note: one byte padding here

#ifdef UNIT_TESTING
friend class TextBufferTests;
Expand All @@ -177,6 +178,12 @@ class TextAttribute final
#endif
};

// The number 14 is historical/arbitrary but verifies the assumptions documented earlier in the comments
// about size and alignment of the fields of the class. The offsetof() macro does not work with private
// data members so about the best we can do is verify that the entire pile of fields adds up to the
// same number=.
static_assert(sizeof(TextAttribute) == 14);

enum class TextAttributeBehavior
{
Stored, // use contained text attribute
Expand All @@ -186,11 +193,15 @@ enum class TextAttributeBehavior

constexpr bool operator==(const TextAttribute& a, const TextAttribute& b) noexcept
{
// Perform these comparisons in the "natural"/"physical" order so that the generated code will
// load and compare entire cache lines in order. The && operator is short-circuiting so
// reordering for aesthetics can and does force the compiler to generate code that will not
// perform optimally.
return a._wAttrLegacy == b._wAttrLegacy &&
a._hyperlinkId == b._hyperlinkId &&
a._foreground == b._foreground &&
a._background == b._background &&
a._extendedAttrs == b._extendedAttrs &&
a._hyperlinkId == b._hyperlinkId;
a._extendedAttrs == b._extendedAttrs;
}

constexpr bool operator!=(const TextAttribute& a, const TextAttribute& b) noexcept
Expand Down