Skip to content

fix: non-type template (enum) instantiation for templ func #175

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.

Sign up for GitHub

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

Merged
Vipul-Cariappa merged 1 commit into from
Dec 1, 2025
Merged

fix: non-type template (enum) instantiation for templ func #175

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions clingwrapper/src/clingwrapper.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -605,6 +605,19 @@ bool split_comma_saparated_types(const std::string& name,
return true;
}

Cpp::TCppScope_t GetEnumFromCompleteName(const std::string &name) {
std::string delim = "::";
Copy link
Collaborator

@vgvassilev vgvassilev Nov 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to do string parsing to do a simple-ish lookup.

Copy link
Collaborator Author

@Vipul-Cariappa Vipul-Cariappa Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is required for now. There is already some string parsing through the Cpp::GetScopeFromCompleteName and Cppyy::GetScope, I wanted to just reuse them instead of introducing this new function. But that is not possible. All of the *GetScope* fail to resolve enum constants. If I fix that, then other tests fail, because that is the expected behaviour according to cppyy.

We should eventually be able to reduce string parsing, but not eliminate it entirely.
To reduce the string parsing, we will need to update https://github.com/compiler-research/CPyCppyy/blob/9d95a1954d0285f85087e40d638b48b40558b42d/src/TemplateProxy.cxx#L175-L176 not to construct a string, instead return std::vector<Cpp::TCppType_t>, or something similar. I believe all the necessary type information is already stored as QualType*, we unnecessarily convert QualType* to std::string, and back to QualType* again.
I will open an issue to track it.
But we cannot remove the string parsing completely because users can instantiate templates with string. Example: func["int", "double", "MyClass"](1, 2.0, gbl.MyClass(...))

Copy link
Collaborator Author

@Vipul-Cariappa Vipul-Cariappa Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issue opened at compiler-research/CPyCppyy#146

Copy link
Collaborator

@vgvassilev vgvassilev Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. Let's move forward!

size_t start = 0;
size_t end = name.find(delim);
Cpp::TCppScope_t curr_scope = 0;
while (end != std::string::npos) {
curr_scope = Cpp::GetNamed(name.substr(start, end - start), curr_scope);
start = end + delim.length();
end = name.find(delim, start);
}
return Cpp::GetNamed(name.substr(start, end), curr_scope);
}

// returns true if no new type was added.
bool Cppyy::AppendTypesSlow(const std::string& name,
std::vector<Cpp::TemplateArgInfo>& types, Cppyy::TCppScope_t parent) {
Expand Down Expand Up @@ -668,6 +681,10 @@ bool Cppyy::AppendTypesSlow(const std::string& name,

if (is_integral(i))
integral_value = strdup(i.c_str());
if (Cpp::TCppScope_t scope = GetEnumFromCompleteName(i))
if (Cpp::IsEnumConstant(scope))
integral_value =
strdup(std::to_string(Cpp::GetEnumConstantValue(scope)).c_str());
types.emplace_back(type, integral_value);
}
return false;
Expand Down