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.

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

Always use discriminant marker in ZeroTrie serialized form #4562

Closed
sffc opened this issue Jan 31, 2024 · 2 comments · Fixed by #4938
Closed

Always use discriminant marker in ZeroTrie serialized form #4562

sffc opened this issue Jan 31, 2024 · 2 comments · Fixed by #4938
Assignees
Labels
C-zerovec Component: Yoke, ZeroVec, DataBake T-techdebt Type: ICU4X code health and tech debt

Comments

@sffc
Copy link
Member

sffc commented Jan 31, 2024

Currently the discriminant marker is only used in the ZeroTrie enum. It is not used in ZeroTrieSimpleAscii, etc. This is potentially error-prone since a ZeroTrie created from one type can be cast into another type with no way to detect the error in most cases until a debug assertion is hit. It would be safer to write out the discriminant in all cases even when the enum is not used. It also means it is easier to generalize from a specific type to a general type and vice-versa.

Should do after #4031 and before 1.5

@sffc sffc added T-techdebt Type: ICU4X code health and tech debt C-zerovec Component: Yoke, ZeroVec, DataBake labels Jan 31, 2024
@sffc sffc added this to the 1.5 Blocking ⟨P1⟩ milestone Jan 31, 2024
@sffc sffc self-assigned this Jan 31, 2024
@sffc sffc added the discuss Discuss at a future ICU4X-SC meeting label Feb 29, 2024
@sffc
Copy link
Member Author

sffc commented Feb 29, 2024

Discuss with:

Optional:

@sffc
Copy link
Member Author

sffc commented Mar 11, 2024

Resolution: OK to do this if it impacts only Serde and is reasonably backwards compatible.

@sffc sffc assigned sffc and unassigned sffc and robertbastian Mar 11, 2024
@sffc sffc assigned sffc and unassigned sffc May 23, 2024
robertbastian pushed a commit that referenced this issue May 28, 2024
…tently (#4938)

Fixes #4562

There are diffs; good to get this into 1.5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-zerovec Component: Yoke, ZeroVec, DataBake T-techdebt Type: ICU4X code health and tech debt
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants