-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[cxxmodules] Preload more modules #1697
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
Conversation
|
Starting build on |
|
@phsft-bot build just on slc6/gcc62 with flags -Druntime_cxxmodules=On -Dctest_test_exclude_none=on |
|
Starting build on |
|
Build failed on slc6/gcc62. Errors:
|
f2131ab to
7b3e73d
Compare
|
Starting build on |
|
Build failed on slc6/gcc62. Errors:
And 23 more |
ROOT can't autoparse classes inside namespaces with the rootmap system (as the loading callbacks don't correctly land where they are supposed to land with our injected namespaces). As this turns out to be a feature of some kind, let's preload TMVA/TreePlayer/Graf to fix all failing tests that are related to this feature/bug with modules enabled. This commit can be dropped if we solve on of those problems: 1. figure out how to fix this bug in the rootmap-based loading without regressin in performance. 2. replace the rootmap system with something else like attaching all C++ modules on startup. Note that we already do something like this in normal ROOT by including these packages into the PCH which also makes those decls available in the normal clang lookup.
Obviously the removal of this preload is a necessary precursor to making the use of module the default. Do we have a mechanism to 'not' forget to remove it? |
|
@pcanal, can you elaborate a little because I cannot understand both sentences ;) |
7b3e73d to
a900091
Compare
|
Starting build on |
|
Build failed on slc6/gcc62. Errors:
And 352 more Warnings:
|
@vgvassilev I was apparently mistakenly talking about 'preloading the library' rather than what you seem to have meant ('preloading the module'). Since it is 'only' the module and that we already do this pre-loading of the AST node via the PCH, removing this 'pre-attaching of the AST nodes via preloading the module' is indeed not a pre-requisite to making the use of module the default mechanism. [It is rather a question of optimization and/or implementation of the replacement for the rootmap forward declaration]. |
|
@phsft-bot build just on slc6/gcc62 with flags -Druntime_cxxmodules=On -Dctest_test_exclude_none=on |
|
Starting build on |
|
I think we can close this as it's out of date and we have a new patch for loading all the modules. Loading more modules have the negative effect on performance, so we're not going to this direction for now. |
|
Which is the new patch you have in mind? |
|
@phsft-bot build just on slc6/gcc62 with flags -Druntime_cxxmodules=On -Dctest_test_exclude_none=on |
|
Starting build on |
|
Build failed on slc6/gcc62. Errors:
And 26 more Warnings:
|
3ed20f2 to
f5eed82
Compare
|
Starting build on |
|
Build failed on slc6/gcc62. Errors:
Warnings:
|
|
Starting build on |
|
@phsft-bot build just on slc6/gcc62 with flags -DROOT_PARALLEL_LINK_JOBS=2 -Druntime_cxxmodules=On -Dctest_test_exclude_none=on |
|
Starting build on |
|
Build failed on slc6/gcc62. Errors:
Warnings:
|
|
@phsft-bot build just on slc6/gcc62 with flags -DROOT_PARALLEL_LINK_JOBS=2 -Druntime_cxxmodules=On -Dctest_test_exclude_none=on |
|
Starting build on |
|
Build failed on slc6/gcc62. Errors:
Warnings:
|
|
@phsft-bot build just on slc6/gcc62 with flags -DLLVM_BUILD_TYPE=Debug -Droofit=Off -Druntime_cxxmodules=On -Dctest_test_exclude_none=On |
|
Starting build on |
|
Build failed on slc6/gcc62. Errors:
|
7d6f3d6 to
3b8336f
Compare
|
Starting build on |
|
Build failed on slc6/gcc62. Errors:
|
Currently RooFit has a bug and we cannot use it with modules builds.
3b8336f to
390cc01
Compare
|
Starting build on |
|
Build failed on slc6/gcc62. Errors:
And 23 more Warnings:
|
|
@Teemperor, I'd like to close this PR but the change to the clang::SourceManager seems still relevant. Could you open a phab review for it? |
|
This is implemented in a different way in the master. |
Rebase PR #1396.
Original PR comment:
"ROOT can't autoparse classes inside namespaces with the rootmap
system (as the loading callbacks don't correctly land where
they are supposed to land with our injected namespaces). As this
turns out to be a feature of some kind, let's preload
TMVA/TreePlayer/Graf to fix all failing tests that are related
to this feature/bug with modules enabled.
This commit can be dropped if we solve on of those problems:
Note that we already do something like this in normal ROOT by
including these packages into the PCH which also makes those
decls available in the normal clang lookup."