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

Some dict's idxHeader struct aren't packed in MSVC/Windows #1576

Closed
shenlebantongying opened this issue Jun 14, 2024 · 3 comments · Fixed by #1785
Closed

Some dict's idxHeader struct aren't packed in MSVC/Windows #1576

shenlebantongying opened this issue Jun 14, 2024 · 3 comments · Fixed by #1785

Comments

@shenlebantongying
Copy link
Collaborator

shenlebantongying commented Jun 14, 2024

Previous discussion #1573 (comment)

All idxHeader have gcc/clang's __attribute((packed))__, however, only some have its counterpart #pragma pack in MSVC.

This is not an issue because all data fields are currently 32 bits, MSVC will likely align the struct to 4 bytes, but this is unspecific/undefined behaviour. No idea if this already leads to bugs, or it will lead to bugs in the future, (or there is a standardized rule that I don't know and this is not a problem at all 😅 ).

https://github.com/search?q=repo%3Axiaoyifang%2Fgoldendict-ng%20idxHeader&type=code

Need more study to know if we can add alignas(4) for all idxHeader to ensure correctness.

@shenlebantongying
Copy link
Collaborator Author

Both clang & gcc supports #pragma pack(push, 1) -> #pragma pack(pop) since ages ago. There are no needs to do it twice, btw.

static_assert(alignof(S) == 1) can ensure extra correctness.

@xiaoyifang
Copy link
Owner

xiaoyifang commented Sep 25, 2024

IMO, the pack feature should be used for cross-platform data migration. For example , move the windows index data to linux or vice versa.

@shenlebantongying
Copy link
Collaborator Author

shenlebantongying commented Sep 25, 2024

cross-platform data migration

This feature needs "portable mode" or

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants