This repository was archived by the owner on Aug 5, 2026. It is now read-only.
Resolve templated type names as a whole in GetScope - #213
Merged
Conversation
guitargeek
force-pushed
the
tmva_test_issue
branch
2 times, most recently
from
June 27, 2026 22:11
a9fcc5b to
b4c6cd0
Compare
Author
|
Failures are pre-existing failures also in other PRs like #212 |
vgvassilev
reviewed
Jun 28, 2026
|
|
||
| std::vector<Cpp::TemplateArgInfo> types; | ||
| InterOpMutex.unlock(); // unlock to allow AppendTypesSlow | ||
| bool no_new_type = Cppyy::AppendTypesSlow(name, types); |
Collaborator
There was a problem hiding this comment.
Suggested change
| bool no_new_type = Cppyy::AppendTypesSlow(name, types); | |
| bool added_new_type = !Cppyy::AppendTypesSlow(name, types); |
maybe that makes the reading slightly more straight-forward.
vgvassilev
reviewed
Jun 28, 2026
| // std), retry with the parent's qualification prepended. | ||
| if (no_new_type && !parent_name.empty()) { | ||
| types.clear(); | ||
| no_new_type = Cppyy::AppendTypesSlow(parent_name + "::" + name, types); |
Collaborator
There was a problem hiding this comment.
We need to figure out how to avoid concatenating strings here.
Author
There was a problem hiding this comment.
Good point! I moved the string concatenation to AppendTypesSlow, so that we're sure the string concatenation is localized to that function.
vgvassilev
reviewed
Jun 28, 2026
| // std), retry with the parent's qualification prepended. | ||
| if (no_new_type && !parent_name.empty()) { | ||
| types.clear(); | ||
| no_new_type = Cppyy::AppendTypesSlow(parent_name + "::" + name, types); |
Collaborator
There was a problem hiding this comment.
Suggested change
| no_new_type = Cppyy::AppendTypesSlow(parent_name + "::" + name, types); | |
| no_new_type = Cppyy::AppendTypesSlow(name, types, /*parent=*/parent_scope); |
I believe this is what you meant?
Author
There was a problem hiding this comment.
Only that this doesn't work with the current implementation of AppendTypesSlow (see other comment)
guitargeek
force-pushed
the
tmva_test_issue
branch
2 times, most recently
from
June 28, 2026 08:50
b0198e3 to
62c042c
Compare
GetScope used to split a templated name like "std::array<float, 3>" into the template "std::array" plus the argument list "float, 3", then feed that list to the all-types __Cppyy_AppendTypesSlow trampoline and instantiate the template from the result. That cannot represent non-type template arguments: the trampoline takes only types, so an argument such as the `3` in std::array<float, 3> forces a guaranteed compile failure. The failure is normally swallowed, but for arguments the string-parsing fallback can't recover (e.g. an enum constant printed as "(EOp)0"), the type fails to resolve and the resulting interpreter error state breaks the next, unrelated JIT call wrapper. Resolve the full type expression through the trampoline instead and read its scope back with GetScopeFromType, so non-type arguments are handled and no failing instantiation is emitted. The trampoline declares its variable in the global scope, so a name written relative to a parent scope (e.g. "vector<int>" looked up in std) would no longer be found. Honor the parent that AppendTypesSlow already accepts: when the name doesn't resolve as given, retry the trampoline declaration with the parent's qualification prepended. Naming a type as a template argument does not instantiate it, so the resolved specialization may be declared but undefined. Earlier the InstantiateTemplate call completed it; restore that guarantee with an explicit IsComplete, since callers expect a complete scope (e.g. to walk its base classes) and would otherwise hit an assertion on an incomplete class.
guitargeek
force-pushed
the
tmva_test_issue
branch
from
June 28, 2026 08:51
62c042c to
a2f913c
Compare
vgvassilev
reviewed
Jun 28, 2026
vgvassilev
left a comment
vgvassilev
left a comment
Collaborator
There was a problem hiding this comment.
Lgtm! Let’s wait for @Vipul-Cariappa for the lock/unlock part.
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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
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.
GetScope used to split a templated name like "std::array<float, 3>" into the template "std::array" plus the argument list "float, 3", then feed that list to the all-types __Cppyy_AppendTypesSlow trampoline and instantiate the template from the result. That cannot represent non-type template arguments: the trampoline takes only types, so an argument such as the
3in std::array<float, 3> forces a guaranteed compile failure. The failure is normally swallowed, but for arguments the string-parsing fallback can't recover (e.g. an enum constant printed as "(EOp)0"), the type fails to resolve and the resulting interpreter error state breaks the next, unrelated JIT call wrapper.Resolve the full type expression through the trampoline instead and read its scope back with GetScopeFromType, so non-type arguments are handled and no failing instantiation is emitted.
Enables the test added in this PR: