-
Notifications
You must be signed in to change notification settings - Fork 7
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.
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
fix: non-type template (enum) instantiation for templ func #175
Conversation
|
Added the test to compiler-research/cppyy#164 |
| } | ||
|
|
||
| Cpp::TCppScope_t GetEnumFromCompleteName(const std::string &name) { | ||
| std::string delim = "::"; |
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.
Why do we need to do string parsing to do a simple-ish lookup.
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.
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(...))
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.
Issue opened at compiler-research/CPyCppyy#146
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.
Sounds good. Let's move forward!
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!
requires test:
This was discovered while working on ROOT workshop slides.