Skip to content

print contents of sentinel-terminated pointers in std.fmt #3972

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

Closed
wants to merge 6 commits into from

Conversation

daurnimator
Copy link
Contributor

Not sure if we need to make error messages more explicit? Would be a good place to use #1669 (comment)

@daurnimator daurnimator added the standard library This issue involves writing Zig code for the standard library. label Dec 22, 2019
@@ -28,7 +28,7 @@ test "cstr fns" {

fn testCStrFnsImpl() void {
testing.expect(cmp("aoeu", "aoez") == -1);
testing.expect(mem.len(u8, "123456789") == 9);
testing.expect(mem.len(u8, "123456789".*) == 9);
Copy link
Member

Choose a reason for hiding this comment

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

The way the test was before is testing what it intends to test - calculating the length of a null terminated pointer. e.g. the zig equivalent of strlen.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should it perhaps be .ptr instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The type of a string literal is *const [9:0]u8; IMO asking for the length of a pointer seems weird, and we need to .* to make sense...

@fengb fengb mentioned this pull request Jan 8, 2020
10 tasks
@LemonBoy
Copy link
Contributor

LemonBoy commented Feb 1, 2020

ping, @daurnimator any chance you could rebase this and address Andrew's comments?

@daurnimator daurnimator force-pushed the fmt-sentinel-pointers branch from 28dfb1e to 1fc9432 Compare February 2, 2020 02:58
@daurnimator
Copy link
Contributor Author

Rebased and addressed some comments.

I went with a new function mem.pointerToSlice where you pass the result type that you want: that result type is used to know the sentinel as well as the const-ness (thereby avoiding the need for two functions: one for const, one for non-const as we currently have).

Copy link
Member

@andrewrk andrewrk left a comment

Choose a reason for hiding this comment

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

I'm not ready for any of these changes to std.mem. Can you just make pointerToSlice a private function in fmt.zig, or, better yet, inline the logic?

Here's a new set of functions to std.mem I would be willing to add (and adding deprecation notices to toSlice, toSliceConst, and len):

/// Takes a pointer to an array, a sentinel-terminated pointer, or a slice, and
/// returns a slice. If there is a sentinel on the input type, there will be a
/// sentinel on the output type. The constness of the output type matches
/// the constness of the input type.
pub fn Slice(comptime T: type) type {
    // TODO
}

/// Takes a pointer to an array, a sentinel-terminated pointer, or a slice, and
/// returns a slice. If there is a sentinel on the input type, there will be a
/// sentinel on the output type. The constness of the output type matches
/// the constness of the input type.
pub fn slice(ptr: var) Slice(@TypeOf(ptr)) {
    // TODO
}

/// Takes a pointer to an array, an array, a sentinel-terminated pointer,
/// or a slice, and returns the length.
/// TODO deprecate `len` and make this become the new `len`.
pub fn length(ptr: var) usize {
    // TODO
}

I believe then std.mem.slice would take the place of pointerToSlice in the fmt.zig code.

The rest of the PR looks good. The changes to std.mem are my only concern.

@daurnimator
Copy link
Contributor Author

Can you just make pointerToSlice a private function in fmt.zig, or, better yet, inline the logic?

How would this then help with #4304 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
standard library This issue involves writing Zig code for the standard library.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants