Skip to content

Implement modules support#15

Merged
ednolan merged 26 commits into
bemanproject:mainfrom
yesmanchyk:yesmanchyk_modules
May 13, 2026
Merged

Implement modules support#15
ednolan merged 26 commits into
bemanproject:mainfrom
yesmanchyk:yesmanchyk_modules

Conversation

@yesmanchyk

@yesmanchyk yesmanchyk commented May 10, 2026

Copy link
Copy Markdown
Contributor

This implementation is based on bemanproject/exemplar#380

There are some key differences, however:

  1. I use import beman.scan_view; instead of #include <beman/scan_view/scan.hpp> in tests and examples.
  2. I had to include all dependencies library feature-test macros into global module fragment because import std; resulted in compilation errors when I tried gcc 16 and clang 21.
  3. Due to above 2 reasons I extracted the beman::scan_view implementation into scan_view.hpp without any changes.

The cmake --build build step takes longer when configured with -DBEMAN_SCAN_VIEW_USE_MODULES=On - here is what Jason Merrill from Red Hat replied about slowdown in gcc 16.1:
I imagine it takes longer because it has to build module std, which includes the whole standard library, before building anything else. I don't know if there's a simple way to teach cmake to share std.gcm between projects/builds.

@yesmanchyk yesmanchyk requested a review from Mick235711 as a code owner May 10, 2026 20:13
Comment thread examples/complex_example.cpp
yesmanchyk and others added 2 commits May 10, 2026 16:22
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Comment thread tests/beman/scan_view/basic.test.cpp Outdated
Comment thread .github/workflows/ci_tests.yml
Comment thread examples/basic_example.cpp Outdated
Comment thread include/beman/scan_view/scan.hpp
Comment thread infra/cmake/enable-experimental-import-std.cmake
Comment thread infra/cmake/enable-experimental-import-std.cmake Outdated
Comment thread include/beman/scan_view/scan_view.cppm Outdated
#include <optional>
#include <ranges>
#include <type_traits>
#include <utility>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Putting standard library includes in the global module fragment is not the approved modules mechanism for this project. To merge this, you'll need to figure out how to get import std building. The best approach would be to do so by building with a supported container or with beman-local-ci.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I solved it in yesmanchyk@f196c4b. However, the line 7 looks like a hack to me even though CI is passing. Maybe it's worth restoring the global module fragment with a single #include <version> inside? I don't know a way to export library feature-test macros with import std;.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

One way is import <version>, but I'm not sure header units are an approved mechanism, nor am I sure all targets support header units well enough...

As mentioned below, it seems that #include <version> (and things like <cstdarg>, <cassert>) is now allowed as a special exception in the GMF.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

import <version>; fails compilation with the following:

error: failed to read compiled module: No such file or directory
note: compiled module file is 
'gcm.cache/./usr/lib/gcc/aarch64-unknown-linux-gnu/16/include/g++-v16/version.gcm'
note: imports must be built before being imported

plus I'm not sure macros will be visible that way

@ednolan ednolan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Great work! Just a few more small comments. By the way, you can use pre-commit run --all-files as a convenient way to apply clang-tidy and ensure that the lint check passes.

Comment thread include/beman/scan_view/scan_view.cppm Outdated
Comment thread include/beman/scan_view/scan_view.cppm Outdated
Comment thread tests/beman/scan_view/basic.test.cpp Outdated

@Mick235711 Mick235711 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks a lot for working on this!

Comment thread include/beman/scan_view/config_generated.hpp.in
Comment thread include/beman/scan_view/scan.hpp
Comment thread CMakeLists.txt
@yesmanchyk

Copy link
Copy Markdown
Contributor Author

Looks like I've addressed all comments - please merge if it looks good.

@yesmanchyk yesmanchyk requested review from Mick235711 and ednolan May 13, 2026 00:26
@ednolan

ednolan commented May 13, 2026

Copy link
Copy Markdown
Member

Great job!

@ednolan ednolan merged commit efaa427 into bemanproject:main May 13, 2026
78 checks passed
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.

3 participants