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

Make Duration constructors and accessors const fns #33033

Closed
wants to merge 1 commit into from

Conversation

sfackler
Copy link
Member

Closes #33029

r? @alexcrichton

@GuillaumeGomez
Copy link
Member

GuillaumeGomez commented Apr 16, 2016

Thanks!

r=me

@luqmana
Copy link
Member

luqmana commented Apr 16, 2016

@bors: r=GuillaumeGomez

@bors
Copy link
Contributor

bors commented Apr 16, 2016

📌 Commit 379c40f has been approved by GuillaumeGomez

@alexcrichton
Copy link
Member

@bors: r-

@GuillaumeGomez this is a change which has ramifications on the stability of these APIs and how we might change them in the future, I'd like to discuss this at a libs triage meeting before we move forward with this just yet.

We currently don't have many conventions for what is or what isn't a const fn in the standard library just yet, and we have many possible candidates for doing so, so I want to just make sure we approach this in a principled fashion rather than "let's make everything const which can be so right now"

@GuillaumeGomez
Copy link
Member

@alexcrichton: My bad, I didn't think that the change would be that big. I'll wait for your come back on this.

@steveklabnik
Copy link
Member

steveklabnik commented Apr 16, 2016

@luqmana to be clear, @GuillaumeGomez doesn't quite have r+ rights yet. I've been working with him on smaller things, but I'm supposed to approve them before they get put in. (and even that is docs only, not code). He forgot to CC me on this one.

I'm surprised Bors allowed this, actually.

@luqmana
Copy link
Member

luqmana commented Apr 16, 2016

@steveklabnik Ah whoops, my mistake. Actually, I don't think bors cares what comes after r= as long as the person actually writing the comment is allowed to.

@sfackler sfackler added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Apr 17, 2016
@@ -81,7 +83,7 @@ impl Duration {
/// nanoseconds are not represented in the returned value).
#[stable(feature = "duration", since = "1.3.0")]
#[inline]
pub fn as_secs(&self) -> u64 { self.secs }
pub const fn as_secs(&self) -> u64 { self.secs }
Copy link
Contributor

Choose a reason for hiding this comment

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

These kind of const fns only work in statics, they don't work in array lengths and other true constants (yet). Is there a use case for this? If not, I suggest to delay this until we have a unified MIR-powered const evaluator

@alexcrichton
Copy link
Member

The libs team discussed this PR during triage today and the decision was to close for now. Adding a const fn is a serious API commitment that we may not always be able to uphold (e.g. #33072). In the meantime const fn is still unstable, and when stabilized we will likely want to perform a pass over the standard library to audit for functions which are const-compatible at that time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants