Skip to content

Fix Issue 9433 - Deprecate delete #1941

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
wilzbach merged 1 commit into from
Feb 19, 2018
Merged

Fix Issue 9433 - Deprecate delete #1941

wilzbach merged 1 commit into from
Feb 19, 2018

Conversation

@JinShil
Copy link
Contributor

@JinShil JinShil commented Dec 5, 2017

Spec and documentation update to accompany dlang/dmd#7342

@dlang-bot
Copy link
Contributor

dlang-bot commented Dec 5, 2017

Thanks for your pull request, @JinShil!

Bugzilla references

Auto-close Bugzilla Description
✓ 9433 Deprecate delete

@JinShil JinShil mentioned this pull request Dec 5, 2017
Merged
@MetaLang
Copy link
Member

MetaLang commented Dec 5, 2017

Even though delete has been "deprecated" for forever, I think this still needs approval from @andralex.

@JinShil JinShil mentioned this pull request Jan 13, 2018
Merged
@JinShil
Copy link
Contributor Author

JinShil commented Feb 8, 2018

This is ready for additional scrutiny and potential merge, now that dlang/druntime#2037 is in.

@JinShil JinShil added this to the 2.079.0 milestone Feb 8, 2018
@wilzbach
Copy link
Contributor

wilzbach commented Feb 8, 2018

FYI: When I deprecated the hex strings, I removed them from the spec immediately (#2118), which led to an outcry on the forums.
So while it's deprecated, we should probably just note that's deprecated and direct the users to destroy or __delete

@JinShil
Copy link
Contributor Author

JinShil commented Feb 9, 2018 •
edited
Loading

So while it's deprecated, we should probably just note that's deprecated and direct the users to destroy or __delete

OK. all the documentation about delete in expression.dd remains, with an added note to prefer object.destroy or __delete, but other places in the documentation where delete's use was advocated, I either removed it or replaced it with object.destroy.

wilzbach
wilzbach reviewed Feb 9, 2018
View reviewed changes
Copy link
Contributor

@wilzbach wilzbach left a comment

Choose a reason for hiding this comment

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

There's REF1_ALTTEST too 🏋️‍♂️

spec/class.dd Outdated
$(P The program can explicitly inform the garbage collector that an
object is no longer referred to (with the delete expression), and
object is no longer referred to
(with $(LINK2 phobos/object.html#.destroy, $(D object.destroy()))), and
Copy link
Contributor

@wilzbach wilzbach Feb 9, 2018

Choose a reason for hiding this comment

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

`$(REF1 destroy, object)
or if you like the parenthesis sth. like $(REF1_ALTTEXT, $(D object.destroy()), destroy, object)

spec/class.dd Outdated
This rule does not apply to auto objects or objects deleted
with the $(I DeleteExpression), as the destructor is not being run
This rule does not apply to auto objects or objects destructed
with $(LINK2 phobos/object.html#.destroy, $(D object.destroy())),
Copy link
Contributor

@wilzbach wilzbach Feb 9, 2018

Choose a reason for hiding this comment

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

ditto

spec/expression.dd Outdated
$(D delete) $(GLINK UnaryExpression)
)
$(P NOTE: `delete` has been deprecated. Instead, please use $(REF1 destroy, object)
if feasible, or $(REF __delete, core, memory) as a last resort)
Copy link
Contributor

@wilzbach wilzbach Feb 9, 2018

Choose a reason for hiding this comment

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

End with .?

@JinShil
Copy link
Contributor Author

JinShil commented Feb 9, 2018

Anyone know what's causing this error in DAutoTest?

LaTeX Warning: Hyper reference `delete_expressions' on page 163 undefined on in
put line 7533.

! You can't use `macro parameter character #' in horizontal mode.
l.7540 ... please use <a href="phobos/object.html#
                                                  .destroy">\lstinline|objec...

? 
! Emergency stop.
l.7540 ... please use <a href="phobos/object.html#
                                                  .destroy">\lstinline|objec...

!  ==> Fatal error occurred, no output PDF file produced!

wilzbach
wilzbach reviewed Feb 9, 2018
View reviewed changes
spec/class.dd
$(P The program can explicitly inform the garbage collector that an
object is no longer referred to with $(REF1 destroy, object), and
then the garbage collector calls the destructor immediately. The
destructor is guaranteed to never be called twice.
Copy link
Contributor

@wilzbach wilzbach Feb 9, 2018

Choose a reason for hiding this comment

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

Missing parenthesis here.

Copy link
Contributor Author

@JinShil JinShil Feb 9, 2018

Choose a reason for hiding this comment

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

The parens is there on the next line: 639.

@wilzbach
Copy link
Contributor

wilzbach commented Feb 9, 2018

Anyone know what's causing this error in DAutoTest?

Yeah, it's the universally beloved DDoc system - it throws no error on trailing parenthesis, but LaTeX will.

@wilzbach wilzbach mentioned this pull request Feb 9, 2018
Merged
@wilzbach
Copy link
Contributor

wilzbach commented Feb 9, 2018

Ah that's the real issue:

l.7540 ... please use <a href="phobos/object.html#
                                                  .destroy">\lstinline|objec...

Fix: #2194

@JinShil
Copy link
Contributor Author

JinShil commented Feb 11, 2018

Thanks to @wilzbach this is now passing and is ready for additional scrutiny, and potentially a merge.

@JinShil
Copy link
Contributor Author

JinShil commented Feb 14, 2018

Ping. The DMD PR has been merged, and this really needs to go in along with it for the next release.

@JinShil
Copy link
Contributor Author

JinShil commented Feb 19, 2018

@andralex or whoever can act on this. This needs to go in with the other, already merged, PRs.

wilzbach
wilzbach approved these changes Feb 19, 2018
View reviewed changes
Copy link
Contributor

@wilzbach wilzbach left a comment

Choose a reason for hiding this comment

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

I typically don't merge spec PRs until @andralex or @WalterBright have given their approval, but as @JinShil pointed out this sat too long in the queue. We need to be able to move faster.

@wilzbach wilzbach merged commit 363eb2d into dlang:master Feb 19, 2018
@andralex
Copy link
Member

andralex commented Feb 20, 2018

Cool, thanks.

@JinShil JinShil mentioned this pull request Mar 18, 2018
Merged
@JinShil JinShil deleted the deprecate_delete branch April 3, 2019 13:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@andralex andralex Awaiting requested review from andralex andralex is a code owner

@WalterBright WalterBright Awaiting requested review from WalterBright WalterBright is a code owner

1 more reviewer

@wilzbach wilzbach wilzbach approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

@andralex Bug Fix Needs Approval

Projects

None yet

Milestone

2.079.0

Development

Successfully merging this pull request may close these issues.

5 participants

@JinShil @dlang-bot @MetaLang @wilzbach @andralex