Skip to content

fix(C++ modules) fix stdmodule priority#6809

Merged
waruqi merged 7 commits intoxmake-io:devfrom
Arthapz:fix-stdmodule-priority
Sep 26, 2025
Merged

fix(C++ modules) fix stdmodule priority#6809
waruqi merged 7 commits intoxmake-io:devfrom
Arthapz:fix-stdmodule-priority

Conversation

@Arthapz
Copy link
Member

@Arthapz Arthapz commented Sep 15, 2025

fixes #6794

@Arthapz Arthapz marked this pull request as draft September 15, 2025 01:51
@Arthapz Arthapz changed the title Fix stdmodule priority fix(C++ modules) fix stdmodule priority Sep 15, 2025
@Arthapz Arthapz force-pushed the fix-stdmodule-priority branch from f8b7cd0 to c403203 Compare September 15, 2025 08:01
@waruqi
Copy link
Member

waruqi commented Sep 16, 2025

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request addresses an issue with C++ standard module priority by correctly caching stdmodules based on the C++ standard library in use. This is a significant improvement for projects that utilize multiple toolchains. The changes also introduce new tests to validate this behavior and refactor some existing module support code for better organization. My review focuses on enhancing the new test scripts for clarity and robustness, and simplifying some of the newly introduced logic for better maintainability.

@Arthapz Arthapz force-pushed the fix-stdmodule-priority branch 3 times, most recently from b6579d4 to 98f86e3 Compare September 17, 2025 06:32
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request fixes an issue with stdmodule priority when using multiple runtimes by caching stdmodule information per C++ library. The core logic change in xmake/rules/c++/modules/support.lua correctly isolates the caches. The pull request also includes a new test case to verify this fix, along with some refactoring and bug fixes in existing test files. The changes look solid and effectively address the problem. I have one suggestion to improve the readability and maintainability of the new test file by reducing code duplication.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request correctly fixes an issue with C++ standard module caching when multiple runtimes are used by scoping the cache to the specific C++ library implementation. The change is supported by refactoring a helper function to a shared location and adding a new test case to verify the fix. The overall changes are well-structured and improve the robustness of C++ module support. I have one suggestion to improve a new test file by reducing code duplication and improving consistency.

@Arthapz Arthapz force-pushed the fix-stdmodule-priority branch 3 times, most recently from 39ce9c2 to 4f44e3c Compare September 17, 2025 08:37
@Arthapz Arthapz force-pushed the fix-stdmodule-priority branch 10 times, most recently from 35a1e91 to a3b5597 Compare September 24, 2025 17:48
@Arthapz Arthapz force-pushed the fix-stdmodule-priority branch 2 times, most recently from 2559764 to c83393a Compare September 24, 2025 19:47
@Arthapz Arthapz marked this pull request as ready for review September 24, 2025 21:03
@Arthapz Arthapz force-pushed the fix-stdmodule-priority branch from c83393a to bcc3c56 Compare September 25, 2025 15:51
@waruqi
Copy link
Member

waruqi commented Sep 26, 2025

Does it ready now?

@Arthapz
Copy link
Member Author

Arthapz commented Sep 26, 2025

Does it ready now?

Yes

1 similar comment
@Arthapz
Copy link
Member Author

Arthapz commented Sep 26, 2025

Does it ready now?

Yes

@waruqi waruqi merged commit 7c9fbf8 into xmake-io:dev Sep 26, 2025
26 of 28 checks passed
@waruqi waruqi added this to the v3.0.4 milestone Sep 26, 2025
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.

Xmake choose wrong C++ std modules when project have targets occupying different toolchains

2 participants