Skip to content

Conversation

@Geod24
Copy link
Member

@Geod24 Geod24 commented Jan 16, 2020

See the changelog entry for the timeline.

I wanted to apply the same to class allocator, but @disable new() was not deprecated, with the rationale that aggregates could want to force to use a different allocator.
It sounds very odd that this very specific things could be allowed. There are other ways to do so (e.g. a factory function which accepts a delegate). But that means another deprecation period for them.
CC @JinShil

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @Geod24!

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

{
semanticTypeInfo(sc, tb);
break;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

This code was a bit surprising: If ad.aggDelete is false (and it should, because otherwise we error out), the if is always followed and the code that follows is never executed. However the code seemed to account for the possibility of f or fd being null, which doesn't make sense as fd can never be null. Maybe it's just historical baggage.

@Geod24 Geod24 force-pushed the class-deallocator branch from c6c094b to d1685b3 Compare January 16, 2020 06:55
@JinShil
Copy link
Contributor

JinShil commented Jan 16, 2020

I wanted to apply the same to class allocator, but @disable new() was not deprecated, with the rationale that aggregates could want to force to use a different allocator. It sounds very odd that this very specific things could be allowed.

Yes, it is odd. @disable new was not deprecated because it was a technique being employed by at least one project in the test suite. It's possible that the 1 or 2 cases where it was being used no longer exist, so it may be possible to deprecate now, but some users were advocating for it to remain (see #8042 (review)), so I left it in.

This is an unfortunate consequence of not following through on the deprecations -- users will choose to use these odd techniques to workaround limitations in the language, which then requires D to support and maintain the feature, or to go through a disproportionate amount of work to get its removal approved.

@Geod24
Copy link
Member Author

Geod24 commented Jan 16, 2020

Fix for the Buildkite failure: libmir/mir-random#121

@Geod24 Geod24 force-pushed the class-deallocator branch from d1685b3 to 3747bbd Compare January 16, 2020 10:47
Copy link
Member

@WalterBright WalterBright left a comment

Choose a reason for hiding this comment

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

It's nice to remove a large chunk of code.

@Geod24
Copy link
Member Author

Geod24 commented Jan 18, 2020

Green

@dlang-bot dlang-bot merged commit c52d5a3 into dlang:master Jan 18, 2020
@Geod24 Geod24 deleted the class-deallocator branch January 18, 2020 13:45
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.

5 participants