[cxxmodules] Fix autoloading of nested namespaces. #1337
Merged
+33
−1
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Starting build on |
|
This PR should supersede PR #1328. |
pcanal
approved these changes
Nov 15, 2017
vgvassilev
force-pushed
the
FixAutoloadingTDF
branch
from
71c7b14 to
bdf6c47
Compare
|
Starting build on |
vgvassilev
force-pushed
the
FixAutoloadingTDF
branch
from
bdf6c47 to
e74bfdd
Compare
|
Starting build on |
|
Build failed on mac1012/native. Warnings:
And 10 more Failing tests:
And 2 more |
vgvassilev
force-pushed
the
FixAutoloadingTDF
branch
from
e74bfdd to
5a9d4bc
Compare
|
Starting build on |
|
Build failed on slc6/gcc49. Failing tests: |
|
Build failed on centos7/gcc49. Failing tests: |
|
@phsft-bot build with -Druntime_cxxmodules=On |
|
Starting build on |
|
Build failed on centos7/gcc49. Failing tests: |
vgvassilev
force-pushed
the
FixAutoloadingTDF
branch
from
5a9d4bc to
da3621d
Compare
|
Starting build on |
|
@phsft-bot just on slc6/gcc62 with flags -Druntime_cxxmodules=On |
|
@phsft-bot build just on slc6/gcc62 with flags -Druntime_cxxmodules=On |
|
Starting build on |
vgvassilev
force-pushed
the
FixAutoloadingTDF
branch
from
da3621d to
b77d6c0
Compare
|
Starting build on |
|
Build failed on slc6/gcc62. Warnings:
And 7 more Failing tests:
And 37 more |
When we want to autoload contents from namespaces we end up in Sema::LookupQualifiedName; then we issue a callback to FindExternallyVisibleName which forwards to LookupObject. Lookup object takes a DeclContext as an argument. This argument is always the primary lookup context (which for a NamespaceDecl is the original namespace. Regular autoloading does not consider this (or has chosen not to) because this reduces the amount of autoloads. Such autoloads can happen when resolving template specializations when computing a decl's linkage by clang's CodeGen. This in turn loads unexpected libraries such as RooFit when trying to resolve all template specializations of __to_raw_pointer (located in <memory>), including the one taking a HistFactory::Data*. That way we end up needlessly loading RooFit and showing it's weird banner, potentially breaking a lot of tests. This behavior can be considered as broken because we hide information about possible redeclarations which can affect the linkage computation or other checks in codegen. If we fix the bug we will probably explode ROOT's memory footprint and make the gap between standard ROOT and ROOT with modules even bigger. Since it is not clear how much work and issue resolving is required for standard ROOT, we can probably only live with the workaround of the missing concept: moving entities in namespaces whose autoloading requires declarations to be in the PCH. For instance, ROOT::Experimental::TDataFrame.
vgvassilev
force-pushed
the
FixAutoloadingTDF
branch
from
b77d6c0 to
2b0e363
Compare
|
Starting build on |
|
Build failed on slc6/gcc62. Warnings:
And 7 more Failing tests:
And 37 more |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When we want to autoload contents from namespaces we end up in
Sema::LookupQualifiedName; then we issue a callback to
FindExternallyVisibleName which forwards to LookupObject. Lookup object
takes a DeclContext as an argument. This argument is always the primary
lookup context (which for a NamespaceDecl is the original namespace.
Regular autoloading does not consider this (or has chosen not to) because
this reduces the amount of autoloads. Such autoloads can happen when
resolving template specializations when computing a decl's linkage by
clang's CodeGen. This in turn loads unexpected libraries such as RooFit
when trying to resolve all template specializations of __to_raw_pointer
(located in <memory>), including the one taking a HistFactory::Data*.
The amount of deserializations might be reduced by applying D29951.
That way we end up needlessly loading RooFit and showing it's weird
banner, potentially breaking a lot of tests.
This behavior can be considered as broken because we hide information
about possible redeclarations which can affect the linkage computation or
other checks in codegen. If we fix the bug we will probably explode
ROOT's memory footprint and make the gap between standard ROOT and ROOT
with modules even bigger.
Since it is not clear how much work and issue resolving is required for
standard ROOT, we can probably only live with the workaround of the missing
concept: moving entities in namespaces whose autoloading requires declarations
to be in the PCH. For instance, ROOT::Experimental::TDataFrame.