Skip to content

Change builtins to return a string literal #8636

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

Merged
merged 6 commits into from
Jun 20, 2021
Merged

Conversation

jmc-88
Copy link
Contributor

@jmc-88 jmc-88 commented Apr 28, 2021

Fixes issue #3779 (I think?).

Copy link
Member

@Vexu Vexu left a comment

Choose a reason for hiding this comment

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

@tagName and @errorName should both return a null terminated slice instead since they can be used with runtime known arguments and returning different types based on whether they are being executed at comptime or not is bad for generic code.

@marler8997
Copy link
Contributor

Looks like @andrewrk was the one to suggest that we use pointers to arrays when the length is comptime known (see #3779 (comment)). I can see how this could cause some issues with generic code, but I think sometimes this information is necessary, so I don't see much choice other than to expose it.

@Vexu
Copy link
Member

Vexu commented Apr 29, 2021

I assumed he was referring to @embedFile and @typeName since they are always comptime but I could be wrong.

@jmc-88
Copy link
Contributor Author

jmc-88 commented Apr 29, 2021

Looks like @andrewrk was the one to suggest that we use pointers to arrays when the length is comptime known (see #3779 (comment)).

Yes, I took that comment as reference, and probably misunderstood it, and here we are. 😄

@tagName and @errorName should both return a null terminated slice instead since they can be used with runtime known arguments and returning different types based on whether they are being executed at comptime or not is bad for generic code.

Just for extra n00b-friendly clarity, the desire is to ensure these builtins all return [:0]const u8?

@Vexu
Copy link
Member

Vexu commented May 29, 2021

This change should also apply to all the strings in std.builtin.TypeInfo, and yes [:0]const u8 is the correct type.

@Vexu
Copy link
Member

Vexu commented Jun 8, 2021

Are you still interested in finishing this pr? I'd be happy to do it for you if not.

@jmc-88
Copy link
Contributor Author

jmc-88 commented Jun 8, 2021

Sorry, yeah, I'd like to work on this. I got busy with real life and now I have a scary rebase in front of me 😄

I'll update this PR as soon as possible and ping the thread.

@Vexu Vexu added the stage1 The process of building from source via WebAssembly and the C backend. label Jun 13, 2021
@Vexu Vexu marked this pull request as draft June 13, 2021 07:48
jmc-88 and others added 6 commits June 16, 2021 21:56
This was already the case, but the documentation failed to point out
that the returned value is of type `*const [N:0]u8`, i.e. that of a
string literal.

Also adds a behavioral test to assert that this is the case.
This is for consistency with the documentation on sentinel-terminated
{arrays,slices,pointers} which already use `N` for a comptime-inferred
size rather than `X`.

Also adds a behavioral test to assert that a string literal is returned.
@jmc-88 jmc-88 marked this pull request as ready for review June 16, 2021 21:11
@jmc-88 jmc-88 requested a review from Vexu June 16, 2021 21:12
@jmc-88
Copy link
Contributor Author

jmc-88 commented Jun 16, 2021

I have updated the PR and it seems to pass test-behavior locally, PTAL.

Copy link
Member

@Vexu Vexu left a comment

Choose a reason for hiding this comment

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

Looks good so far, do you also want to make the change to @typeInfo and @src?

@jmc-88
Copy link
Contributor Author

jmc-88 commented Jun 19, 2021

Looks good so far, do you also want to make the change to @typeInfo and @src?

I have local changes ready to update string fields contained in structures returned by these two builtins to [:0]const u8.

But while applying changes to @typeInfo() I realized its std.builtin.TypeInfo output is also an input to @Type() and that could break existing code. For instance, this assignment fails since it uses a []const u8 as returned by std.mem.tokenize() and that can't implicitly cast to [:0]const u8:

.name = name,

Would you say this is WAI?

EDIT: to clarify, this seems to make it impossible to use a similar pattern where std.mem.tokenize() is used at comptime, unless there's a way to do comptime memory allocations for ensuring the slice is null-terminated.

@Vexu
Copy link
Member

Vexu commented Jun 20, 2021

Hmm, I didn't think of that. I see three ways to solve this:

  1. Have separate types for @typeInfo and @Type, which would break even more code.
  2. Leave things as they are, which kind of defeats the purpose of this change.
  3. Add casts from []T -> [:t]T and *[N]T -> *[N:t]T when the value is comptime known, which would make this change purely cosmetic.

I'll make an issue for this since this pr is already useful as is.

@Vexu Vexu merged commit 6de45c8 into ziglang:master Jun 20, 2021
@jmc-88 jmc-88 deleted the issue-3779 branch June 20, 2021 21:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stage1 The process of building from source via WebAssembly and the C backend.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants