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.

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

Rename createOne to new #1873

Closed
wants to merge 4 commits into from

Conversation

Projects
None yet
3 participants
@daurnimator
Copy link
Contributor

commented Jan 6, 2019

For #1872

daurnimator added some commits Jan 6, 2019

@andrewrk

This comment has been minimized.

Copy link
Member

commented Jan 6, 2019

This should probably also rename destroy to be delete.

Show resolved Hide resolved test/tests.zig Outdated
@daurnimator

This comment has been minimized.

Copy link
Contributor Author

commented Jan 6, 2019

This should probably also rename destroy to be delete.

I don't know about that; I don't see it as necessary for this PR.
Also in another open PR I have a change in that area: b3528a6

@andrewrk

This comment has been minimized.

Copy link
Member

commented Jan 6, 2019

I don't know about that; I don't see it as necessary for this PR.

We're talking about the API for allocation, right?

Status quo: create / destroy
After this change: new / destroy

Isn't delete the word that goes with new?

@daurnimator

This comment has been minimized.

Copy link
Contributor Author

commented Jan 7, 2019

Status quo: create / destroy

Really it's createOne / destroy. My desire was only to make it a single word, I'd be fine renaming it to create but that would cause upgrade issues.

We also have free which is almost the same as destroy.

@daurnimator daurnimator force-pushed the daurnimator:new branch 2 times, most recently from e1c9404 to 588473c Jan 7, 2019

daurnimator added some commits Jan 6, 2019

@daurnimator daurnimator force-pushed the daurnimator:new branch from 588473c to bc8701b Jan 7, 2019

@mgxm

This comment has been minimized.

Copy link
Contributor

commented Jan 7, 2019

I tried to reproduce the failed build on FreeBSD without success I don't think it's related to this PR.
https://builds.sr.ht/~mgxm/job/20414

We could resubmit the build again and see if it works.

@daurnimator

This comment has been minimized.

Copy link
Contributor Author

commented Jan 7, 2019

btw, this latest revision is no longer a breaking change; so please remove the label @andrewrk

@andrewrk andrewrk removed the breaking label Jan 7, 2019

@andrewrk

This comment has been minimized.

Copy link
Member

commented Feb 3, 2019

I think it's too early for this change. Let's revisit this along with other naming changes closer to 1.0.0.

I definitely would want to either keep it as create/destroy or new/delete but not something in between.

@andrewrk andrewrk closed this Feb 3, 2019

andrewrk added a commit that referenced this pull request Feb 3, 2019

`std.mem.Allocator.create` replaced with better API
`std.mem.Allocator.createOne` is renamed to `std.mem.Allocator.create`.

The problem with the previous API is that even after copy elision,
the initalization value passed as a parameter would always be a copy.
With the new API, once copy elision is done, initialization
functions can directly initialize allocated memory in place.

Related:
 * #1872
 * #1873
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.