Skip to content

Fix potential endianness and alignment problems with Span-based UnicodeTrie constructor - #284

Merged
JimBobSquarePants merged 1 commit into
SixLabors:mainfrom
DaZombieKiller:unicode-trie-fix
Jun 30, 2022
Merged

Fix potential endianness and alignment problems with Span-based UnicodeTrie constructor#284
JimBobSquarePants merged 1 commit into
SixLabors:mainfrom
DaZombieKiller:unicode-trie-fix

Conversation

@DaZombieKiller

Copy link
Copy Markdown
Contributor

Prerequisites

  • I have written a descriptive pull-request title
  • I have verified that there are no overlapping pull-requests open
  • I have verified that I am following matches the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
  • I have provided test coverage for my change (where applicable)

Description

This is a small PR making some changes to the recently-introduced ReadOnlySpan<byte> constructor for UnicodeTrie. Currently, the data provided to this constructor is constant data generated via the Roslyn ROS<byte> = new byte[] trick, which doesn't guarantee alignment on the data. The data is also stored in little-endian, which requires swapping for big-endian platforms.

  • UnicodeTrieHeader is now a simple structure containing only fields, and it no longer has a StructLayout attribute. Its layout won't result in padding, so Pack = 1 was removed.
  • The UnicodeTrie constructor now reads UnicodeTrieHeader with MemoryMarshal.Read, to ensure there are no problems with alignment on platforms that require it (MemoryMarshal.Read performs an unaligned read.)
  • The rawData span is no longer Casted to uint but rather the destination uint array is casted to byte, for similar alignment-related reasons.
  • When the platform is not little-endian, the constructor will now swap the endianness of the data before interpreting it.

@CLAassistant

CLAassistant commented Jun 30, 2022

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@JimBobSquarePants

Copy link
Copy Markdown
Member

Haha... You beat me by seconds. Was just committing to a branch with your changes! 😁

@JimBobSquarePants JimBobSquarePants left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Brilliant. Thanks! 👍

@JimBobSquarePants JimBobSquarePants added this to the 1.0.0-rc1 milestone Jun 30, 2022
@codecov

codecov Bot commented Jun 30, 2022

Copy link
Copy Markdown

Codecov Report

Merging #284 (1c6bd54) into main (fc97cb1) will decrease coverage by 0%.
The diff coverage is 22%.

@@          Coverage Diff          @@
##            main    #284   +/-   ##
=====================================
- Coverage     83%     83%   -1%     
=====================================
  Files        222     222           
  Lines      12200   12199    -1     
  Branches    1756    1759    +3     
=====================================
- Hits       10167   10159    -8     
- Misses      1606    1610    +4     
- Partials     427     430    +3     
Flag Coverage Δ
unittests 83% <22%> (-1%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/SixLabors.Fonts/Unicode/UnicodeTrie.cs 88% <22%> (-12%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fc97cb1...1c6bd54. Read the comment docs.

@JimBobSquarePants
JimBobSquarePants merged commit 863ac8c into SixLabors:main Jun 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants