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.

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

Make GCAllocator usable in pure functions #6432

Merged
merged 1 commit into from
Apr 8, 2018
Merged

Make GCAllocator usable in pure functions #6432

merged 1 commit into from
Apr 8, 2018

Conversation

nordlow
Copy link
Contributor

@nordlow nordlow commented Apr 7, 2018 •
edited

Qualify members of GCAllocator as const to make it usable in pure contexts.

Because new can be used in pure contexts I see no reason why GCAllocator shouldn't be that aswell.

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @nordlow! 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 + phobos#6432"

wilzbach
wilzbach reviewed Apr 7, 2018
View reviewed changes
std/experimental/allocator/gc_allocator.d Outdated
@@ -1,6 +1,6 @@
// Written in the D programming language.
/**
D's built-in garbage-collected allocator.
D's built-in garbage-collected allocator.
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@nordlow nordlow force-pushed the pure-gc-allocator branch 2 times, most recently from 2e6008b to 056b644 Compare April 7, 2018 16:15
@nordlow
Copy link
Contributor Author

nordlow commented Apr 7, 2018

Had to update a static assert aswell.

n8sh
n8sh reviewed Apr 7, 2018
View reviewed changes
std/experimental/allocator/gc_allocator.d
@@ -117,31 +117,31 @@ struct GCAllocator
are `shared`.
*/

static shared GCAllocator instance;
static shared const GCAllocator instance;
Copy link
Member

Choose a reason for hiding this comment

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

Why not immutable? Does it matter one way or the other?

Copy link
Contributor Author

@nordlow nordlow Apr 8, 2018 •
edited

Choose a reason for hiding this comment

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

No, AFAICT. An immutable data is automatically shared, but since GCAllocator contains no data members I don't think it matters in this case.

Update: Apparently, it must be const. Changing to immutable fails as

std/experimental/allocator/building_blocks/segregator.d(377): Error: non-shared method std.experimental.allocator.building_blocks.segregator.Segregator!(128LU, GCAllocator, GCAllocator).Segregator.Impl!().goodAllocSize is not callable using a shared object

Copy link
Member

Choose a reason for hiding this comment

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

That's bizarre.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@andralex @edi33416 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

The issue seems to be that when you have static immutable GCAllocator instance the following
is(typeof(GCAllocator.instance) == shared) is false, which is odd.

If you have a look at Segregator it compares the small and large allocators against shared in order to decide how to mixin the Impl template struct.

With immutable, the is expression is false and thus the shared attribute is not applied.
I don't know why it's false; maybe a compiler bug? @andralex

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably a new PR would be better

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll try that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@edi33416, can you have a go at this? My time is currently very spare.

Copy link
Contributor

Choose a reason for hiding this comment

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

@nordlow I'll have at it after DConf

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks.

@nordlow nordlow force-pushed the pure-gc-allocator branch 2 times, most recently from 2df3cb1 to 3c438cd Compare April 8, 2018 18:17
n8sh
n8sh reviewed Apr 8, 2018
View reviewed changes
std/experimental/allocator/typed.d Outdated
@@ -22,7 +22,7 @@ import std.traits : isPointer, hasElaborateDestructor;
import std.typecons : Flag, Yes, No;

/**
Allocation-related flags dictated by type characteristics. `TypedAllocator`
Allocation-related flags dictated by type characteristics. `TypedAllocator`
Copy link
Member

Choose a reason for hiding this comment

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

No reason for indent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@n8sh n8sh added the auto-merge label Apr 8, 2018
@dlang-bot dlang-bot merged commit 36f549a into dlang:master Apr 8, 2018
@nordlow
Copy link
Contributor Author

nordlow commented Apr 8, 2018

Thanks.

@wilzbach
Copy link
Member

wilzbach commented Apr 9, 2018

Sorry for being away over the weekend, but I thought we couldn't do this:

The current code models that reality. Yes, we did make a concession for new to allow pure functions to allocate, but I don't want to use that as precedent to essentially model things that don't exist.

I suggest we leave the low-level allocators alone. Then use a cast to force makeArray be pure.

#4733 (comment)

(I tried the same ~ two years ago)

@nordlow
Copy link
Contributor Author

nordlow commented Apr 9, 2018

Thanks, anyway.

@wilzbach
Copy link
Member

wilzbach commented Apr 9, 2018

In case I wasn't clear: I was trying to say that except @andralex changed his mind, I think we need to revert this :/

@andralex
Copy link
Member

andralex commented Apr 9, 2018

After working on allocators and collection for a good while, I got to the conclusion the allocation funcs must be pure, so let's keep this in.

@nordlow
Copy link
Contributor Author

nordlow commented Apr 9, 2018

Great, MmapAllocator will be next!

@wilzbach wilzbach mentioned this pull request Apr 9, 2018
Merged
@nordlow nordlow deleted the pure-gc-allocator branch October 19, 2021 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@edi33416 edi33416 edi33416 left review comments

@wilzbach wilzbach wilzbach left review comments

@n8sh n8sh n8sh approved these changes

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

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

Assignees
No one assigned
Labels
auto-merge
Projects
None yet
Milestone
No milestone
6 participants
@nordlow @dlang-bot @wilzbach @andralex @edi33416 @n8sh