From 4b57fdf3e2eb8a7ba76d57765572652dde4f0a4b Mon Sep 17 00:00:00 2001
From: Axel Naumann <Axel.Naumann@cern.ch>
Date: Mon, 30 Apr 2018 14:34:19 +0200
Subject: [PATCH] Revert "D40901 (e51a2b9de4) allows us to autoload canonical
namespaces."
This reverts commit 7fa206c4db87c0b14b771ffa41188189d8849647.
It introduces a spurious load of libRooFit, visible in the failure of
projectroot.roottest.root.treeproxy.roottest_root_treeproxy_make
---
core/metacling/src/TCling.cxx | 26 ++++++++++++++++++++++++++
1 file changed, 26 insertions(+)
diff --git a/core/metacling/src/TCling.cxx b/core/metacling/src/TCling.cxx
index a645533af5e..5a58d3a2240 100644
--- a/core/metacling/src/TCling.cxx
+++ b/core/metacling/src/TCling.cxx
@@ -5039,11 +5039,37 @@ namespace {
// because it may shadow the lookup of other names contained
// in that namespace
+ #ifdef R__USE_CXXMODULES
+ // 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 instantiations 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.
+ //
+ // FIXME: We might want to consider enabling this for regular autoloading once we have a good understanding
+ // of the performance implications.
NamespaceDecl* nsOrigDecl = nsDecl->getOriginalNamespace();
if (nsDecl != nsOrigDecl) {
nsOrigDecl->setHasExternalVisibleStorage();
fNSSet.insert(nsOrigDecl);
}
+ #endif
nsDecl->setHasExternalVisibleStorage();
fNSSet.insert(nsDecl);
return true;
--
GitLab