Skip to content

Fix Issue 18228 - this(this a){} doesn't generate postblit ctor; this(this){} does #8141

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

Jump to bottom
Merged
merged 1 commit into from
Apr 19, 2018
Merged

Fix Issue 18228 - this(this a){} doesn't generate postblit ctor; this(this){} does #8141

merged 1 commit into from
Apr 19, 2018

Conversation

@RazvanN7
Copy link
Contributor

@RazvanN7 RazvanN7 commented Apr 5, 2018

No description provided.

@dlang-bot
Copy link
Contributor

dlang-bot commented Apr 5, 2018 •
edited
Loading

Thanks for your pull request and interest in making D better, @RazvanN7! 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 annotated coverage diff directly on GitHub with CodeCov's browser extension
  • 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
✓ 18228 normal this(this a){} doesn't generate postblit ctor; this(this){} does

Testing this PR locally

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

dub fetch digger
dub run digger -- build "master + dmd#8141"

@rainers
Copy link
Member

rainers commented Apr 5, 2018

Please also note https://issues.dlang.org/show_bug.cgi?id=12228 which shows that the syntax has been explicitly added at some point, but with very strange limitations. This is not something that is specific to constructors but should be banned in any case, but Walter disagreed in that bug report.

@rainers
Copy link
Member

rainers commented Apr 11, 2018

andralex approved these changes 17 hours ago

@andralex Please approve removing using this and super as a type, too, please. See https://issues.dlang.org/show_bug.cgi?id=12228

I suspect changing this needs a deprecation first, though.

MetaLang
MetaLang suggested changes Apr 11, 2018
View reviewed changes
test/fail_compilation/fail18228.d Outdated
TEST_OUTPUT:
---
fail_compilation/fail18228.d(11): Error: `this` cannot be used as a parameter type, use `typeof(this)` instead
fail_compilation/fail18228.d(12): Error: `this` cannot be used as a parameter type, use `typeof(this)` instead
Copy link
Member

Choose a reason for hiding this comment

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

This is a comma splice. Please change the comma to a semi-colon or period.

Copy link
Member

Choose a reason for hiding this comment

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

@MetaLang do you have the rights to make changes to PRs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Thanks!

andralex
andralex approved these changes Apr 12, 2018
View reviewed changes
Copy link
Member

@andralex andralex left a comment

Choose a reason for hiding this comment

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

OK with me. I think we need a deprecation.

@andralex
Copy link
Member

cc @WalterBright

@JinShil
Copy link
Contributor

JinShil commented Apr 12, 2018 •
edited
Loading

OK with me. I think we need a deprecation.

@andralex If that comment was in response to @rainers comment about removing this and super as a type, please explicitly state your approval in the issue tracker at https://issues.dlang.org/show_bug.cgi?id=12228

@RazvanN7 RazvanN7 force-pushed the Issue_18228 branch 2 times, most recently from 77e56f6 to b2e34be Compare April 12, 2018 08:38
@RazvanN7
Copy link
Contributor Author

OK with me. I think we need a deprecation.

Made it a deprecation.

jacob-carlborg
jacob-carlborg suggested changes Apr 12, 2018
View reviewed changes
Copy link
Contributor

@jacob-carlborg jacob-carlborg left a comment

Choose a reason for hiding this comment

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

A changelog is required as well, describing the problem and what to do to solve it.

@RazvanN7
Copy link
Contributor Author

@jacob-carlborg Done.

jacob-carlborg
jacob-carlborg reviewed Apr 12, 2018
View reviewed changes
changelog/deprecate_this_as_param_type.dd
//Use `typeof(this)` instead

this(int a, this b) {} // ditto
}
Copy link
Contributor

Choose a reason for hiding this comment

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

There's no information how to resolve it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, I guess it's mentioned in the deprecation message which is included above.

@jacob-carlborg
Copy link
Contributor

Do we need a spec update? How are we dealing with deprecations and spec updates?

@dlang-bot dlang-bot merged commit bbc087e into dlang:master Apr 19, 2018
@JinShil JinShil mentioned this pull request May 12, 2018
Merged
@JinShil JinShil mentioned this pull request Jun 2, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@andralex andralex andralex approved these changes

+3 more reviewers

@MetaLang MetaLang MetaLang requested changes

@jacob-carlborg jacob-carlborg jacob-carlborg approved these changes

@JinShil JinShil JinShil approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Merge:auto-merge Severity:Bug Fix

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@RazvanN7 @dlang-bot @rainers @andralex @JinShil @jacob-carlborg @MetaLang