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

Fix C++20 global module fragment inclusion #1047

Merged
merged 2 commits into from
Aug 2, 2023
Merged

Fix C++20 global module fragment inclusion #1047

merged 2 commits into from
Aug 2, 2023

Conversation

DonutVikingChap
Copy link
Contributor

@DonutVikingChap DonutVikingChap commented Jul 31, 2023

Using the experimental C++20 modules support in recent builds of Clang, EnTT currently causes compilation errors when included as part of the global module fragment in a module interface unit that exports part of EnTT's API and is then imported and used by a different translation unit. Here is a minimal example (note the "error: no matching function for call to 'popcount'" in the output): https://godbolt.org/z/9vTjz9Prh

The reason seems to be that certain function declarations that are implicitly required by the implementation aren't exported into the module interface since they are erroneously marked 'static' and have internal linkage when they should (presumably) be 'inline'. I found that changing these declarations accordingly fixed the problem and made EnTT work well with C++20 modules in my project, and this PR reflects those changes.

Unfortunately I can't write proper test cases for this since it depends on features that are still experimental in most compilers, so there may still be other static declarations that I have missed that could cause problems in other modules-based projects, but as long as these changes don't break anything for regular non-modules builds, they should hopefully at least help make EnTT a bit more modules-friendly in the short term.

@skypjack skypjack self-requested a review August 1, 2023 07:10
@skypjack skypjack self-assigned this Aug 1, 2023
@skypjack skypjack added the enhancement accepted requests, sooner or later I'll do it label Aug 1, 2023
@skypjack
Copy link
Owner

skypjack commented Aug 1, 2023

Makes sense actually, thanks for pointing this out. Why inline though? We can just drop the static and that's it.
Those are template functions, no need to make them inline to avoid symbols duplication. On the other hand, I'm pretty sure that the compiler knows better than me whether to inline them or not.
So, yeah, I'd rather get rid of it if possible before merging. Thanks. 👍

src/entt/core/type_info.hpp Outdated Show resolved Hide resolved
src/entt/core/type_info.hpp Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Aug 1, 2023

Codecov Report

Merging #1047 (1aa76da) into wip (ed64c28) will not change coverage.
Report is 3 commits behind head on wip.
The diff coverage is 100.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@           Coverage Diff           @@
##              wip    #1047   +/-   ##
=======================================
  Coverage   99.98%   99.98%           
=======================================
  Files         142      142           
  Lines       24440    24431    -9     
=======================================
- Hits        24436    24427    -9     
  Misses          4        4           
Files Changed Coverage Δ
src/entt/entity/entity.hpp 100.00% <ø> (ø)
src/entt/core/type_info.hpp 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

@DonutVikingChap
Copy link
Contributor Author

Ok, the inline specifiers are removed now. In my own projects I usually mark all header-defined free functions either inline or constexpr even when they're templates. It's mostly for consistency, but also to make sure that the inline specifier is not accidentally forgotten in case the function is later refactored to not be a template, though I agree that it's not strictly necessary here.

@skypjack
Copy link
Owner

skypjack commented Aug 2, 2023

Yeah, don't get me wrong. What you say makes perfectly sense. I'm just trying to stick with the coding style of this project. 👍
Thanks for changing it. Let me know if you spot any other changes that are useful to make it work with C++20.
I also want to create a C++20 branch of the library soon, most likely after the vacations. 🥳

@skypjack skypjack merged commit f4d52a6 into skypjack:wip Aug 2, 2023
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement accepted requests, sooner or later I'll do it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants