Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork56.4k
Fix JS Ptr factory namespace regression#28213
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:4.x
Are you sure you want to change the base?
Conversation
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.
Pull request overview
This PR fixes a regression in the JavaScript bindings generator where factory methods returningPtr<T> were not being correctly namespace-qualified. The issue stemmed from comparing the C++ class name (inner) with the normalized Python class name (class_info.name), which have different formats and would never match. The fix removes this comparison to restore namespace qualification.
Key Changes:
- Removes the
inner == class_info.namecheck that was preventing namespace qualification from occurring - Ensures unqualified
Ptr<T>return types in factory methods are properly namespace-qualified
💡Add Copilot custom instructions for smarter, more guided reviews.Learn how to get started.
| inner=ret_type[len('Ptr<'):-1].strip() | ||
| if'::'notininnerandinner==class_info.name: | ||
| if'::'notininner: | ||
| ret_type='Ptr<%s>'%class_info.cname |
CopilotAIDec 17, 2025
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.
The unconditional replacement withclass_info.cname could cause incorrect type substitution. When a factory method returns a pointer to a class different from the current class, this will incorrectly use the current class's fully qualified name instead of the returned class's name. Consider using the cname of the actual return type by looking it up:if inner in self.classes: ret_type = 'Ptr<%s>' % self.classes[inner].cname (which is already extracted asbase_class_name on line 514).
| ret_type='Ptr<%s>'%class_info.cname | |
| # Prefer thecname of the actual pointee class if known; fall back to the | |
| # enclosing class's cname to preserve previous behavior. | |
| ifinnerinself.classes: | |
| ret_type='Ptr<%s>'%self.classes[inner].cname | |
| else: | |
| ret_type='Ptr<%s>'%class_info.cname |
vrabaud commentedDec 18, 2025
It's actually that one you need to fix: opencv/modules/js/generator/embindgen.py Line 721 indc52acc
I do not understand the copilot comment fully but it seems valid and works for me.@Shubh3155 , please use it if you think it is valid |
This PR fixes a regression in the JS bindings generator where factory methods
returning Ptr were not correctly namespace-qualified.
The fix restores unconditional namespace qualification when '::' is missing
and ensures the type substitution loop always runs.
Verified locally with:
make gen_opencv_js_source