-
-
Notifications
You must be signed in to change notification settings - Fork 758
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
[Draft] Improve C++20 modules support #2623
Conversation
Thanks, I have some other things I'm working on, maybe I need to spend some time to review and test this patch carefully later, please wait for some time, maybe a few days. |
No problem :3 |
I try it, but it doesn't seem to be compatible with the lower versions vs Z:\personal\xmake\tests\projects\c++\modules\hello>xmake -rv
[ 0%]: generating.cxx.module.deps src\main.cpp
checking for flags (-std:c++20) ... ok
C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.2
9.30133\bin\HostX64\x64\cl.exe /EHsc -nologo -std:c++20 /experimental:module /if
cSearchDir build\.gens\hello\windows\x64\release\rules\modules\cache /ifcSearchD
ir build\.gens\hello\windows\x64\release\stlmodules\cache /stdIfcDir C:\PROGRA~2
\MICROS~1\2019\COMMUN~1\VC\Tools\MSVC\1429~1.301\ifc\x64 /TP /scanDependencies b
uild\.gens\hello\windows\x64\release\rules\modules\cache\src\main.cpp.json src\m
ain.cpp /Fobuild\.objs\hello\windows\x64\release\src\main.cpp.obj
cl : Command line warning D9002 : ignoring unknown option '/scanDependencies'
main.cpp.json
c1xx: fatal error C1083: Cannot open source file: 'build\.gens\hello\windows\x64
\release\rules\modules\cache\src\main.cpp.json': No such file or directory
main.cpp
Generating Code...
error: execv(C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\To
ols\MSVC\14.29.30133\bin\HostX64\x64\cl.exe /EHsc -nologo -std:c++20 /experiment
al:module /ifcSearchDir build\.gens\hello\windows\x64\release\rules\modules\cach
e /ifcSearchDir build\.gens\hello\windows\x64\release\stlmodules\cache /stdIfcDi
r C:\PROGRA~2\MICROS~1\2019\COMMUN~1\VC\Tools\MSVC\1429~1.301\ifc\x64 /TP /scanD
ependencies build\.gens\hello\windows\x64\release\rules\modules\cache\src\main.c
pp.json src\main.cpp /Fobuild\.objs\hello\windows\x64\release\src\main.cpp.obj)
failed(2) |
We do not need to introduce new interfaces, which would break the projects of existing users. Also, we can extract files from rule("c++.build.modules")
set_extensions(".mpp", ".mxx", ".cppm", ".ixx")
after_install(function (target)
local sourcebatch = target:sourcebatches()["c++.build.modules"]
-- TODO install *.mpp
-- sourcebatch.sourcefiles are both
end xmake/xmake/rules/c++/modules/xmake.lua Lines 22 to 23 in e45d940
Support for cppmodules should be done in the rules as far as possible. |
This patch changes the implementation of a lot of c++ modules, but in many places it is not fully compatible with the current xmake architecture. I may refer to its implementation to improve c++ modules, but may not be able to merge it directly. Also, I haven't been able to fully understand some of the improvements in this patches, can you briefly explain them? |
Also, I see that you have added support for the std module, can you add some relevant test examples to the tests directory? |
So, for building modules and header units, we need 3 passes the first pass we will generate all header units, to have knowledge about them, the C++ standard made a common format to get dependencies knowledge https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2022/p1689r5.html these flags generate a json file with a list of artefact that are generated by a module, and their dependencies next we can parse this to generate header units on the second pass we need to sort the modules by dependencies and generate their BMI, but to this right we need to indicate to the compiler how to get module dependencies on MSVC /reference for modules and /headerUnit:{angle/quote} for header units then on the last pass we can build cpp files |
Thanks. |
Now it use add_files and support older version of VS and Clang, and i added headerunit tests (but they will file on Clang < 15, header units is supported only by clang >= 15) |
I have switched this patches to the This is because I need to make further improvements to it, such as code style, internal structure improvements, etc. |
1e6cf01
to
81e4b04
Compare
It should be good now |
5cabcec
to
2af7c3a
Compare
It works for vs2019 now, but when I trying stl_headerunits test, it will raise
|
2af7c3a
to
068ada5
Compare
fixed |
It works now, thanks. |
068ada5
to
6f30805
Compare
6f30805
to
728126b
Compare
I merged this patch to cxxmodules first, then I'll improve it further and test it in detail, and format some code and naming styles. I still need to take some time to understand the implementation and make sure there are no other issues. If you have any other improvements, you can continue to commit them to this branch as well. |
I have opened a new pr and I will continue some reviews on this one |
local headerunits | ||
for _, objectfile in ipairs(batch.objectfiles) do | ||
for obj, m in pairs(modules) do | ||
if obj == objectfile then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not use local m = modules[objectfile]
to reduce for-loop?
batchcmds:show_progress(opt.progress, "${color.build.object}generating.cxx.module.bmi %s", name) | ||
|
||
local bmifile = provide.bmi | ||
table.join2(args, { "-c", "-x", "c++-module", "--precompile", provide.sourcefile, "-o", bmifile }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really understand here, if there are multiple values for provides, then there seems to be some problems.
clang -c sourcefile -o bmifile -c sourcefile2 -o bmifile2 ...?
|
||
batchcmds:add_depfiles(provide.sourcefile) | ||
batchcmds:set_depmtime(os.mtime(bmifile)) | ||
batchcmds:set_depcache(target:dependfile(bmifile)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will be overwritten several times
batchcmds:vrunv(compinst:program(), table.join(compinst:compflags({target = target}), common_args, bmifiles, {"-c", "-o", objectfile})) | ||
|
||
batchcmds:set_depmtime(os.mtime(objectfile)) | ||
batchcmds:set_depcache(target:dependfile(objectfile)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will be overwritten several times
|
||
local bmifile = provide.bmi | ||
if add_module_to_mapper(mapper_file, name, path.absolute(bmifile, project.directory())) then | ||
table.join2(args, { "-c", provide.sourcefile }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and here.
gcc -o objectfile -c sourcefile1 -c sourcefile2 -c sourcefile3 ...?
batchcmds:set_depmtime(os.mtime(bmifile)) | ||
batchcmds:set_depcache(target:dependfile(bmifile)) | ||
|
||
flag = {referenceflag, name .. "=" .. path.filename(bmifile)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will be overwritten several times for each provides
Headerunits doesn't work on Clang <= 15 (which is currently in RC version)
GCC have some issues with header units, but it's compiler bug related
MSVC work pretty well in all my test cases
this PR introduce add_modulefiles, the objective of this API is to offer in the future, module installation like header installation
a lot of improvement can be made on this code as i'm not fully comfortable with Lua and XMake internals