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

Unexpected readLine() / printing behaviour #1055

Closed
ghost opened this issue Jun 5, 2018 · 9 comments
Closed

Unexpected readLine() / printing behaviour #1055

ghost opened this issue Jun 5, 2018 · 9 comments
Labels
enhancement Solving this issue will likely involve adding new logic or components to the codebase.
Milestone

Comments

@ghost
Copy link

ghost commented Jun 5, 2018

pub fn main() !void {
    const std = @import("std");
    const warn = std.debug.warn;    
    const io = std.io;
    var input: [300]u8 = undefined;
    const line_len = try io.readLine(input[0..]);
    warn("{} end", input);   
}

I would expect this code to echo back the content that is entered followed by the string " end". But it also prints the contents of the empty locations in the input variable.
Output -

12345
12345                                                                                                                                                                                                                                                                                                        end

This is real surprising behaviour that you need to do warn("{} end", input[0..line_len]); for it to work as expected.

IMO zig really needs a proper string type as this current behaviour is seriously going backwards even from C.

@BraedonWooding
Copy link
Contributor

BraedonWooding commented Jun 5, 2018

You sure it doesn't work...? Using try it outline it printed what you would expect, you can view it here. (Look under the 'debug' section since its a debug print) Of course if you give it extra spaces in input it'll print those out since that's what has been 'read in'. And note: it'll print out any junk that exists in that space unless '0'd out, because you are asking for the whole buffer to be printed.

Initially it may seem like a string type is needed (I would know I proposed one), however the way Zig approaches things really means you just don't need one, string manipulation can be done with Unicode pretty easily (and I'm almost done with a library to do it easier) :).

If it doesn't print that in the current version of Zig, then that's a compiler bug.

@Hejsil
Copy link
Contributor

Hejsil commented Jun 5, 2018

Also, stdin can accept any sequence of bytes, even invalid UTF8 or ascii, so it would not be correct to return a "string", unless you want readLine to validate the input before giving you the result.

What might be useful though, would be for readLine to return []u8 instead of usize. Then your example becomes:

pub fn main() !void {
    const std = @import("std");
    const warn = std.debug.warn;    
    const io = std.io;
    var input: [300]u8 = undefined;
    const line = try io.readLine(input[0..]);
    warn("{} end", line);   
}

@tiehuis
Copy link
Member

tiehuis commented Jun 5, 2018

So the main difference here versus C is that we don't use null-terminated strings for input, so using the buffer directly isn't going to be a direct analogue like with fgets, where the fgets call is implicitly specifying the length by null terminating the input in the function. We don't null-terminate buffers, so we need to return the length independently to know how much was read instead.

Some other alternative things we can do:

  • Return a slice representing the read buffer. Then you could just do const line = try io.readLine(input[0..]); which may be a little more obvious.
  • Take a growable ArrayList (preferably a Utf8String) and pass that to the function. The length can be changed within it and it matches your expectation.

There a some tradeoffs in these approaches and they are useful in different scenarios. I think the main issue here is that the lack of stdlib documentation/usage for various stdlib functions means there is a barrier for new users.

#965

@ghost
Copy link
Author

ghost commented Jun 5, 2018

Of course if you give it extra spaces in input it'll print those out since that's what has been 'read in'.

And that is the point. The function is named readLine() so it should only read upto a '\n' and therefore the buffer should only contain upto that and therefore only that should be printed. Coming from other languages that is the default behaviour you would expect.

I also agree that it would be best for the function to return a []u8 directly.
const line: []u8 = try io.readLine();

@BraedonWooding
Copy link
Contributor

BraedonWooding commented Jun 5, 2018

... Why would the buffer only contain that though? You set a buffer of 100 long then expect the buffer to change its length on you? To me that just sounds less conventional and obscure. If you want null terminated strings there are std lib functions to convert to them, and you could go that route. I don't know a language that would change the length of a static array?

Also your confusing what the signature would be it would be this;
const line: []u8 = try io.readline(buf[0..]); where you still have to give it a buffer, else how would it create the memory? There is no 'standard malloc' in Zig so either you have to pass it an allocator or a buffer. It would then return a slice of the buffer containing the line :).

@ghost
Copy link
Author

ghost commented Jun 5, 2018

Why would the buffer only contain that though? You set a buffer of 100 long then expect the buffer to change its length on you?

Then have it take a resizable buffer.

@BraedonWooding
Copy link
Contributor

That is a 'solution', but I struggle to see the problem in the first place; why have it take a resizable buffer which is less efficient just to not have to deal with slicing? I think the solution of providing it a buffer and it returning the slice is a more 'elegant' solution that doesn't involve allocating memory along with the reasons others have given.

@andrewrk andrewrk added this to the 0.4.0 milestone Jun 5, 2018
@andrewrk andrewrk added the enhancement Solving this issue will likely involve adding new logic or components to the codebase. label Jun 5, 2018
@andrewrk
Copy link
Member

andrewrk commented Jun 5, 2018

I agree that this is a footgun. Let's update the standard library so that more users do not run into this problem.

@Hejsil
Copy link
Contributor

Hejsil commented Nov 30, 2018

Fixed #1801

@Hejsil Hejsil closed this as completed Nov 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Solving this issue will likely involve adding new logic or components to the codebase.
Projects
None yet
Development

No branches or pull requests

4 participants