Skip to content

Conversation

@JinShil
Copy link
Contributor

@JinShil JinShil commented May 14, 2019

It appears that removing delete is going to be quite difficult. In the meantime, we can at least move forward on class allocators and deallocators.

Spec update: dlang/dlang.org#2640

compilable/vgc1.d(30): vgc: `new` causes a GC allocation
compilable/vgc1.d(31): vgc: `new` causes a GC allocation
compilable/vgc1.d(32): vgc: `new` causes a GC allocation
compilable/vgc1.d(34): vgc: `new` causes a GC allocation
Copy link
Member

Choose a reason for hiding this comment

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

Instead of changing line numbers again, can we use a #line directive for future-proofing ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Updating the test output is fully automated, so this is less work (:

@Geod24
Copy link
Member

Geod24 commented May 14, 2019

Well actually... Can you move the error to the parsing phase ? It would trigger in version(none) & co but that would eventually break anyway.

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @JinShil! 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

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

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#9786"

@JinShil
Copy link
Contributor Author

JinShil commented May 14, 2019

Can you move the error to the parsing phase ?

We still need to allow for @disable new(); Users are using that as a way to disable garbage collected instances.

delete is also not being removed yet.

Given that, is it still possible to emit an error in the parsing phase?

@Geod24
Copy link
Member

Geod24 commented May 14, 2019

Users are using that as a way to disable garbage collected instances.

Do you have a link to some usages ? Wondering if we can replace this by something else.

delete is also not being removed yet.

Sorry, I'm not sure I follow ?

@JinShil
Copy link
Contributor Author

JinShil commented May 14, 2019

Do you have a link to some usages ? Wondering if we can replace this by something else.

https://github.com/Basile-z/iz/blob/7a6b298e161e4d6f8f98fe5fd994e32add70bb18/import/iz/memory.d#L280

Sorry, I'm not sure I follow ?

This PR is just advancing the deprecation of class deallocators...

class C
{
    delete(void* p) { }  // <-- This will now cause the compiler to emit an error.
 }

C c = new C();
delete c;  // <-- We still need to support this.

I'm looking into how the parser works now, but we'll still need to be able to parse delete.

@JinShil
Copy link
Contributor Author

JinShil commented May 14, 2019

@CyberShadow Please see the buildkite output.

sys/data.d(489,13): Error: class allocators are obsolete, consider moving the allocation strategy outside of the class

Can we persuade you to update your code?

@jacob-carlborg
Copy link
Contributor

Can we persuade you to update your code?

Create an issue or PR in the failing project.

CyberShadow added a commit to CyberShadow/ae that referenced this pull request May 14, 2019
@CyberShadow
Copy link
Member

I removed the class allocator/deallocator from ae (it was already deprecated), and pushed a new tag.

@wilzbach
Copy link
Contributor

Do you have a link to some usages ? Wondering if we can replace this by something else.

See also e.g. https://github.com/dlang/phobos/pull/6003/files

@dlang-bot dlang-bot merged commit a938d74 into dlang:master May 15, 2019
@JinShil JinShil deleted the remove_alloc_dealloc branch May 15, 2019 06:51
@Geod24
Copy link
Member

Geod24 commented May 15, 2019

See also e.g. https://github.com/dlang/phobos/pull/6003/files

And Andrei's rebutal includes "this lacks good use cases"
Disabling new seems a rather rare usage, and doesn't fit well with our current attitude of decoupling the type definition from its usage (e.g. we deprecated scope class)

@thewilsonator
Copy link
Contributor

*cough**grumble* see bottom of https://github.com/thewilsonator/Dlang-AGM/blob/master/2019/General.md *cough**grumble*

@JinShil
Copy link
Contributor Author

JinShil commented May 15, 2019

And Andrei's rebutal includes "this lacks good use cases"
Disabling new seems a rather rare usage, and doesn't fit well with our current attitude of decoupling the type definition from its usage (e.g. we deprecated scope class)

I wholeheartedly agree with that, but it's a battle I just can't afford to fight. I'm having a hard enough time getting even more obvious things past the gatekeepers. The easiest way forward for me was to add a special case to allow it and move forward with deprecating everything else.

This highlights why moving forward with deprecations is important, because otherwise, users find creative uses for these deprecated features, and then due to our aversion to code breakage, we get stuck with supporting it. But this kind of work is just too unimportant to @WalterBright and @andralex so it doesn't get done for years, and by then we're forced to make such concessions. Not addressing the little stuff has consequences.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants