-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[cxxmodules] Refactor LoadCoreModules. #1394
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 |
|
Build failed on ubuntu14/native. Errors:
|
|
Build failed on mac1012/native. Errors:
|
|
Build failed on slc6/gcc62. Errors:
|
|
Build failed on slc6/gcc49. Errors:
|
|
Build failed on centos7/gcc49. Errors:
|
fdaf551 to
5b3c6d5
Compare
|
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:
Warnings:
And 7 more Failing tests:
And 274 more |
5b3c6d5 to
9dd4fec
Compare
|
Starting build on |
|
Build failed on slc6/gcc62. Errors:
Warnings:
And 7 more Failing tests:
And 274 more |
9dd4fec to
a49e8db
Compare
a49e8db to
ff9a4ad
Compare
|
Uff, clang format doesn't like the RegisterModule way of doing code formatting... |
|
Starting build on |
ff9a4ad to
bb286fe
Compare
|
Starting build on |
|
Build failed on slc6/gcc62. Errors:
Warnings:
And 7 more Failing tests:
And 48 more |
|
Additional runtime modules failures are because of the (legitimate) warning that we can't find modules sometimes. Will be fixed in a future PR where we add more paths to clang's PrebuiltModulePaths. |
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!
If we generate a dictionary with a C++ module with rootcling we should write in the payload that we generated a C++ module. This way we can actually print a *reliable* warning when we fail to load the C++ module during runtime. Without this we can only speculate if a missing C++ module is intentional.
This refactors the LoadCoreModules function into a LoadModule function that centralizes the loading/diagnosing module loading in TCling. It also now prints out warnings when a module can't be found, which brings diagnostic to the module loading in `RegisterModule`.
97fbd59 to
76d87f1
Compare
|
Starting build on |
|
@phsft-bot build just on slc6/clang_gcc62 with flags -Dcxxmodules=Off -Druntime_cxxmodules=On -Dctest_test_exclude_none=on |
|
Starting build on |
|
Build failed on slc6/clang_gcc62. |
|
@phsft-bot build just on centos7/gcc62 with flags -Druntime_cxxmodules=On -Dctest_test_exclude_none=on |
|
Starting build on |
|
Build failed on centos7/gcc62. |
|
@phsft-bot build just on centos7/gcc62 with flags -Druntime_cxxmodules=On -Dctest_test_exclude_none=on |
|
Starting build on |
|
Build failed on centos7/gcc62. |
|
@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/gcc62 with flags -Druntime_cxxmodules=On -Dctest_test_exclude_none=on -Droofit=Off |
|
Starting build on |
|
@phsft-bot build just on slc6/gcc62 with flags -Druntime_cxxmodules=On -Dctest_test_exclude_none=on -Droofit=Off |
|
Starting build on |
|
Build failed on slc6/gcc62. Errors:
Warnings:
And 7 more Failing tests:
And 55 more |
|
What is the status here? Should this be updated and merged, or closed? |
|
Well, it mostly just exposes a bug more visible (the lack of codegen when loading code with |
|
@Teemperor can you rebase or shall I? |
|
That's PR #1665. |
Refactored LoadCoreModules that it now prints a warning if a module isn't found. Also fixes some nullptr-derefs from the old code where we accessed pointers before checking if they're null.