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

clean: mark all Idxheader packed explicitly #1785

Conversation

shenlebantongying
Copy link
Collaborator

@shenlebantongying shenlebantongying commented Sep 25, 2024

close #1576

This PR does absolutely nothing technically, because removing these packing compiler directives is OK 😅😅😅.

In both 32 & 64 bits machines, a struct with all 32bit fields will be already “packed” (This doesn't appear to be in the standard, but it is what all 3 major compilers do).

However, this PR makes code's purpose clear (Not really, because only after reading the code, one would know that GD is using this trick to read/write multiple fields at once) and reduce inconsistency (It is pretty weird to see the inconsistency as showed in #1576.).


This PR also removes gcc and clang's __attribute__((packed))__ because they both of them supports #pragma pack(push, 1) -> #pragma pack(pop) since ages ago (In fact, clang has a warning about incomplete pair of push/pop).

I tested this code https://godbolt.org/z/YvWvsf9ad for compilers that we care (the one in Debian stable & Ubuntu 22.04 and some older ones). To ensure this certainly works, I added a harmless static_assert for every idxHeader.


This PR corrected 2 inconsistent int in dsl.cc and gsl.cc. This change is correct because based on this table the int type is 32bit anyway (https://en.cppreference.com/w/cpp/language/types#Properties).

@shenlebantongying shenlebantongying force-pushed the fix/explicting-pack-idxheader branch from 1e4076f to 5fe8dd0 Compare September 25, 2024 03:44
@shenlebantongying shenlebantongying force-pushed the fix/explicting-pack-idxheader branch from 5fe8dd0 to d9e6298 Compare September 25, 2024 03:45
@shenlebantongying shenlebantongying force-pushed the fix/explicting-pack-idxheader branch from f95a032 to ed6ca5c Compare September 25, 2024 03:53
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
B Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

@shenlebantongying shenlebantongying changed the title fix: mark all Idxheader packed explicitly and check this fact fix: mark all Idxheader packed explicitly Sep 25, 2024
@shenlebantongying shenlebantongying changed the title fix: mark all Idxheader packed explicitly clean: mark all Idxheader packed explicitly Sep 25, 2024
@shenlebantongying shenlebantongying merged commit 58f346f into xiaoyifang:staged Sep 25, 2024
7 of 8 checks passed
@shenlebantongying shenlebantongying deleted the fix/explicting-pack-idxheader branch September 25, 2024 10:09
@xiaoyifang
Copy link
Owner

what about alignas https://learn.microsoft.com/en-us/cpp/cpp/alignas-specifier?view=msvc-170
seems more elegant.

@shenlebantongying
Copy link
Collaborator Author

shenlebantongying commented Sep 26, 2024

They aren't exact the same, despite that they can often use interchangeably.

See the error in 4th snippet -> https://godbolt.org/z/scMKE1EME (clang says it is an error 👍 . GCC silently ignored it❗. MSVC warns that alignas will be ignored 😅)

  • alignas still follow some rules
  • pack is for violating these rules to make things compact

However, for idxHeader, there is no difference, because not using any of these things is also OK (but that could be risky).

I don't want to change this anymore. The original issue of "inconsistency that oddly looks like a potential source of bug" is fixed.

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 this pull request may close these issues.

Some dict's idxHeader struct aren't packed in MSVC/Windows
2 participants