-
Notifications
You must be signed in to change notification settings - Fork 39
fix look up of shadow constructor #689
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 look up of shadow constructor #689
Conversation
|
clang-tidy review says "All clean, LGTM! 👍" |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #689 +/- ##
==========================================
+ Coverage 79.64% 79.65% +0.01%
==========================================
Files 9 9
Lines 3930 3942 +12
==========================================
+ Hits 3130 3140 +10
- Misses 800 802 +2
🚀 New features to boost your workflow:
|
lib/CppInterOp/CppInterOp.cpp
Outdated
| dyn_cast_or_null<CXXConstructorDecl>(CUSD->getTargetDecl())) { | ||
| CXXConstructorDecl* Result = getSema().findInheritingConstructor( | ||
| SourceLocation(), CXXCD, CUSD); | ||
| // Result is appended to the decls, i.e. CXXRD, iterator |
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 implementation is awkward... Would that mean that the iterators are invalidated?
lib/CppInterOp/CppInterOp.cpp
Outdated
| for (Decl* DI : CXXRD->decls()) { | ||
| if (auto* MD = dyn_cast<DeclType>(DI)) | ||
| methods.push_back(MD); | ||
| else if (auto* USD = dyn_cast<UsingShadowDecl>(DI)) |
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.
I guess we could simplify the levels of indentation if we did:
for (Decl* DI : CXXRD->decls()) {
if (auto* USD = dyn_cast<UsingShadowDecl>(DI))
DI = USD->getTargetDecl();
...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 my approach here did not work?
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.
We want ConstructorUsingShadowDecl to pass to findInheritingConstructor. UsingShadowDecl::getTargetDecl will return NamedDecl, in this case CXXConstructorDecl.
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.
Yes, but then you will do the usual checks with dyn_cast, right?
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.
No, we cannot do it, because we require the original DI later, in line 862.
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.
Oh, ok.
8c19103 to
1a63d3d
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
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!
|
clang-tidy review says "All clean, LGTM! 👍" |
cfab9ca to
95fa63d
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
95fa63d to
ae5947e
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
shadow constructor need to be reconstructed for the base classes
Co-authored-by: Vassil Vassilev <[email protected]>
ae5947e to
0ba1726
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
Description
Shadow constructor need to be reconstructed for the base classes
Fixes # (issue)
Helps fix 1 test in cppyy
Type of change
Please tick all options which are relevant.
Testing
Checklist