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

TAI64 library for std #1400

Closed
wants to merge 2 commits into from
Closed

Conversation

kristate
Copy link
Contributor

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.

Thanks for the implementation.

If this standard is popular enough, I think it makes sense for it to be the type returned by the cross platform abstraction for getting the current time.

// It has two parts: 1) a TAI64 label and 2) an integer, between 0
// and 999999999 inclusive, counting nanoseconds from the
// beginning of the second represented by the TAI64 label.
tai64n: [12]u8,
Copy link
Member

Choose a reason for hiding this comment

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

Why store this encoded? Seems like this should be:

sec: u64,
nsec: u30,

to_bytes can be:

pub fn encode(self: TAI64N) [12]u8 {
    var result: [12]u8 = undefined;
    mem.writeInt(result[0..8], self.sec, builtin.Endian.Big);
    mem.writeInt(result[8..12], self.nsec, builtin.Endian.Big);
    return result;
}

Then we can have a corresponding decode function.

Copy link
Member

Choose a reason for hiding this comment

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

Forgot to add, with #287 this would be guaranteed to not memcpy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is used heavily as a wire format, so it's easy to create the TAI64N by simply grabbing it off the wire -- likewise for placing it back on the wire. The less CPU cycles the better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, I don't know if it is a wise idea to use it platform internal. to reiterate, TAI64N is a wire encoding format (think databases and packet headers).

Copy link
Member

Choose a reason for hiding this comment

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

In general if you want to grab something off the wire and save cpu cycles, then you'll keep it encoded and never decode it. So you'll have a []u8 and we don't need to put it in a struct and give it a name. That's optimal and simpler.

The reason you would want to put it in a struct and give it a name is to do something with the decoded information. For example, from_unixtime(), now(), to_unixtime() all deal with the decoded information.

Equality comparison and ordering comparison can be done directly on the encoded bytes as you demonstrated, and as we do with utf8-encoded strings.

@andrewrk andrewrk modified the milestones: 0.3.0, 0.4.0 Aug 25, 2018
@andrewrk
Copy link
Member

andrewrk commented Sep 18, 2018

I'd like to see this used in a real life zig project before accepting it into the standard library.

My recommendation is to continue working on whatever project it was where you needed this, and include the implementation in your project. Once you get to a certain milestone where we can see your project in action, and we can see how this TAI64 API is used, we can determine whether and how to upstream it into the zig standard library.

@andrewrk andrewrk closed this Sep 18, 2018
@andrewrk andrewrk modified the milestones: 0.4.0, 0.3.0 Sep 28, 2018
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.

2 participants