Skip to content
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

Also expose 'parent' when it's abstract #6876

Merged

Conversation

thaven
Copy link
Contributor

@thaven thaven commented Feb 22, 2019

When generating methods to implement e.g. an interface, it is also very useful to have an alias to the (abstract) function being overridden. E.g. to have access to UDAs.

@dlang-bot
Copy link
Contributor

dlang-bot commented Feb 22, 2019

Thanks for your pull request and interest in making D better, @thaven! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close Bugzilla Severity Description
19715 minor AutoImplement `self` and `parent` aliases are incorrect for overloaded functions

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + phobos#6876"

@thaven thaven force-pushed the enhancement/autoimplement-add-abstract-parent branch 2 times, most recently from 8452b92 to ea157a0 Compare February 22, 2019 16:45
Copy link
Contributor

@thewilsonator thewilsonator left a comment

Choose a reason for hiding this comment

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

Tests please

@thaven
Copy link
Contributor Author

thaven commented Mar 1, 2019

I added a separate test now (and removed the static assert that I originally added in the existing test). The test is a stripped-down version of the actual code I need this change for.

It seems it actually works for non-overloaded functions, but fails for overloaded ones. I think I'll need to loop through the overload set to find the correct overload to alias.

@thaven
Copy link
Contributor Author

thaven commented Mar 1, 2019

It turns out the problem with overloading is an existing bug and also applies to the self alias...

@thaven thaven mentioned this pull request Mar 1, 2019
@thaven
Copy link
Contributor Author

thaven commented Mar 3, 2019

Created Issue 19715 about the self and parent aliases.

Could this PR move on without that fix (by temporarily commenting out an overload in the test + adding a TODO referencing the issue)?

@thewilsonator
Copy link
Contributor

Yes, fixing senate issues separately is usually a good idea.

@thaven thaven force-pushed the enhancement/autoimplement-add-abstract-parent branch from 65bcb11 to a4d1c62 Compare March 5, 2019 06:33
@thaven thaven force-pushed the enhancement/autoimplement-add-abstract-parent branch from a4d1c62 to 47291bb Compare December 3, 2021 13:07
@dlang-bot dlang-bot removed the stalled label Dec 3, 2021
@thaven
Copy link
Contributor Author

thaven commented Dec 3, 2021

Rebased this old PR on current master. Let's see how the tests turn out now. (previous run, long ago, just got stuck somewhere)

@dlang-bot dlang-bot merged commit a86c1eb into dlang:master Jan 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants