-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[cxxmodules] Export macros from modules #1381
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
[cxxmodules] Export macros from modules #1381
Conversation
|
Starting build on |
|
@phsft-bot build just on slc6/gcc62 with flags -Druntime_cxxmodules=On -Dctest_test_exclude_none=on |
|
Starting build on |
vgvassilev
left a comment
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.
LGTM!
|
@phsft-bot build just on slc6/gcc62 with flags -Druntime_cxxmodules=On -Dctest_test_exclude_none=on |
|
Starting build on |
|
@phsft-bot build just on slc6/gcc52 with flags -Druntime_cxxmodules=On -Dctest_test_exclude_none=on |
|
Didn't recognize |
|
@phsft-bot build just on slc6/gcc62 with flags -Druntime_cxxmodules=On -Dctest_test_exclude_none=on |
|
Starting build on |
| // Let's follow the GCC recommendation and undefine `I` in case any of the | ||
| // core modules have defined it: | ||
| // https://www.gnu.org/software/libc/manual/html_node/Complex-Numbers.html | ||
| interp.declare("#ifdef I\n #undef I\n #endif\n"); |
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.
@vgvassilev I reply here to your comment on the closed PR. I don't think we call recursively LoadCoreModule or recursively make TCling instances? And if we do, then we already made a good portion of declares before this code for all the other TCling things, so we would anyway be in deep trouble.
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 think we ought to reduce all declares in TCling and that seems to be an easy-to-reduce one.
52021b0 to
dfe40dd
Compare
In clang there are two records of what modules are visible: One VisibleModuleSet is in the preprocessor, one is in the sema. In the current code we only make every core module visible in sema, but no in the preprocessor. This causes that we can load decls from the core modules with that, but we can't load macros from them as they are still invisible in the preprocessor. This patch just also makes the module visible in the preprocessor.
|
Starting build on |
In commit 1847320 we remove the code that undefs I which breaks some tests since we now again export macros from the STL module.
|
Build failed on slc6/gcc62. Errors:
Warnings:
And 7 more Failing tests:
And 53 more |
|
@phsft-bot build with flags -Druntime_cxxmodules=Off -Dctest_test_exclude_none=off |
|
Starting build on |
|
Build failed on mac1012/native. Failing tests: |
This PR fixes the issue that we don't export macros from the loaded modules. See the specific commits for more.