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

std.mem.Allocator: replace create signature with construct signature #1141

Merged

Conversation

kristate
Copy link
Contributor

@kristate kristate commented Jun 20, 2018

ref #733

/// Call destroy with the result	
/// TODO once #733 is solved, this will replace create	
pub fn construct(self: *Allocator, init: var) Error!*@typeOf(init) {	

std/mem.zig Outdated
/// Alias of create
/// Call destroy with the result
pub fn construct(self: *Allocator, init: var) Error!*@typeOf(init) {
debug.warn("std.mem.Allocator.construct is deprecated;\n");
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

I think we should just remove it. Zig has few users, no big codebases (std is the biggest) and there have already been many breaking changes over the past month :)
Besides, if create's signature is changed anyways, then this is already a breaking change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@andrewrk thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

Oops, saw this just now after I already merged it. I agree with @Hejsil's logic and I removed it in a fixup commit as part of merging. 👍

@@ -193,7 +193,11 @@ fn BaseLinkedList(comptime T: type, comptime ParentType: type, comptime field_na
/// A pointer to the new node.
pub fn allocateNode(list: *Self, allocator: *Allocator) !*Node {
comptime assert(!isIntrusive());
return allocator.create(Node);
return allocator.create(Node{
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

It probably doesn't matter too much, but the right replacement is probably return allocator.create(Node(undefined));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, I was wondering about this!

@andrewrk
Copy link
Member

Looks great! Congrats on your first PR.

@andrewrk andrewrk merged commit 6bd8610 into ziglang:master Jun 20, 2018
@kristate kristate deleted the stdmem-replace-create-with-construct branch June 21, 2018 05:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants