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

Stabilize `Vec::new` and `String::new` as `const fn`s #64028

Merged
merged 2 commits into from Sep 16, 2019

Conversation

@Centril
Copy link
Member

commented Aug 30, 2019

Closes #64022.

r? @oli-obk

/// If you change `RawVec<T>::new` or dependencies, please take care to not
/// introduce anything that would truly violate `min_const_fn`.
///
/// NOTE: We could avoid this hack and check conformance with some

This comment has been minimized.

Copy link
@Centril

Centril Aug 30, 2019

Author Member

I'm pretty unhappy about this hack since I think carries the risk that we might stabilize things we don't want in const eval inadvertently and I have WIP more principled solution in https://github.com/Centril/rust/commits/stabilize-vec-new-const which I think we should seriously consider instead (but which can change into after this PR if necessary).

@Centril

This comment has been minimized.

Copy link
Member Author

commented Aug 30, 2019

cc @alexcrichton for T-libs FCP

@rust-highfive

This comment was marked as resolved.

Copy link
Collaborator

commented Aug 30, 2019

The job x86_64-gnu-llvm-6.0 of your PR failed (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
2019-08-30T23:18:02.5098467Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-08-30T23:18:02.5310827Z ##[command]git config gc.auto 0
2019-08-30T23:18:02.5385295Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-08-30T23:18:02.5457304Z ##[command]git config --get-all http.proxy
2019-08-30T23:18:02.5593775Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/64028/merge:refs/remotes/pull/64028/merge
---
2019-08-30T23:25:08.1691413Z     Finished release [optimized] target(s) in 1m 31s
2019-08-30T23:25:08.1768137Z tidy check
2019-08-30T23:25:09.0952164Z * 578 error codes
2019-08-30T23:25:09.0953468Z * highest error code: E0733
2019-08-30T23:25:09.1583409Z tidy error: /checkout/src/liballoc/raw_vec.rs:125: malformed stability attribute: missing `feature` key
2019-08-30T23:25:10.0936258Z some tidy checks failed
2019-08-30T23:25:10.0942950Z 
2019-08-30T23:25:10.0942950Z 
2019-08-30T23:25:10.0945031Z command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor"
2019-08-30T23:25:10.0945156Z 
2019-08-30T23:25:10.0945216Z 
2019-08-30T23:25:10.0951839Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
2019-08-30T23:25:10.0951924Z Build completed unsuccessfully in 0:01:34
2019-08-30T23:25:10.0951924Z Build completed unsuccessfully in 0:01:34
2019-08-30T23:25:10.0996507Z == clock drift check ==
2019-08-30T23:25:10.1021139Z   local time: Fri Aug 30 23:25:10 UTC 2019
2019-08-30T23:25:10.1833173Z   network time: Fri, 30 Aug 2019 23:25:10 GMT
2019-08-30T23:25:10.1837878Z == end clock drift check ==
2019-08-30T23:25:11.5488226Z ##[error]Bash exited with code '1'.
2019-08-30T23:25:11.5518983Z ##[section]Starting: Checkout
2019-08-30T23:25:11.5520672Z ==============================================================================
2019-08-30T23:25:11.5520742Z Task         : Get sources
2019-08-30T23:25:11.5520785Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@Centril Centril force-pushed the Centril:stabilize-alloc-new-2 branch from 01cbb77 to 9ffe706 Aug 31, 2019

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

@rfcbot fcp merge

@rfcbot

This comment has been minimized.

Copy link

commented Sep 3, 2019

Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot

This comment has been minimized.

Copy link

commented Sep 3, 2019

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot

This comment has been minimized.

Copy link

commented Sep 13, 2019

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

@bors

This comment was marked as resolved.

Copy link
Contributor

commented Sep 14, 2019

☔️ The latest upstream changes (presumably #64456) made this pull request unmergeable. Please resolve the merge conflicts.

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Sep 16, 2019

r=me with a rebase

@Centril Centril force-pushed the Centril:stabilize-alloc-new-2 branch from 9ffe706 to 9b3e11f Sep 16, 2019

@Centril

This comment has been minimized.

Copy link
Member Author

commented Sep 16, 2019

r? @alexcrichton @bors r=alexcrichton rollup

@bors

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2019

📌 Commit 9b3e11f has been approved by alexcrichton

Centril added a commit to Centril/rust that referenced this pull request Sep 16, 2019
Rollup merge of rust-lang#64028 - Centril:stabilize-alloc-new-2, r=al…
…excrichton

Stabilize `Vec::new` and `String::new` as `const fn`s

Closes rust-lang#64022.

r? @oli-obk
bors added a commit that referenced this pull request Sep 16, 2019
Auto merge of #64510 - Centril:rollup-m03zsq8, r=Centril
Rollup of 10 pull requests

Successful merges:

 - #63955 (Make sure interned constants are immutable)
 - #64028 (Stabilize `Vec::new` and `String::new` as `const fn`s)
 - #64119 (ci: ensure all tool maintainers are assignable on issues)
 - #64444 (fix building libstd without backtrace feature)
 - #64446 (Fix build script sanitizer check.)
 - #64451 (when Miri tests are not passing, do not add Miri component)
 - #64467 (Hide diagnostics emitted during --cfg parsing)
 - #64497 (Don't print the "total" `-Ztime-passes` output if `--prints=...` is also given)
 - #64499 (Use `Symbol` in two more functions.)
 - #64504 (use println!() instead of println!(""))

Failed merges:

r? @ghost

@bors bors merged commit 9b3e11f into rust-lang:master Sep 16, 2019

4 checks passed

pr Build #20190916.31 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-6.0) Linux x86_64-gnu-llvm-6.0 succeeded
Details
pr (LinuxTools) LinuxTools succeeded
Details

@Centril Centril deleted the Centril:stabilize-alloc-new-2 branch Sep 16, 2019

// FIXME(mark-i-m): use `cap` when ifs are allowed in const
cap: [0, !0][(mem::size_of::<T>() == 0) as usize],
a: Global,
}

This comment has been minimized.

Copy link
@RalfJung

RalfJung Sep 16, 2019

Member

@ecstatic-morse @oli-obk are you keeping a list somewhere for all the awful hacks we have in libstd to work around const qualif limitations? This would be another one (there's also #63786).

This comment has been minimized.

Copy link
@Centril

Centril Sep 16, 2019

Author Member

Maybe we should have a special string for "const related awful hack"? FIXME(awful_const_hack) :D

We can make an issue also...

This comment has been minimized.

Copy link
@ecstatic-morse

ecstatic-morse Sep 16, 2019

Contributor

We could also just tag PRs like this as const-workaround-if on GitHub, and someone can go through at the tagged PRs once the required operations are allowed.

This comment has been minimized.

Copy link
@Centril

Centril Sep 16, 2019

Author Member

If we're going with a label, I'd make it a bit more general: const-hack.

This comment has been minimized.

Copy link
@ecstatic-morse

ecstatic-morse Sep 16, 2019

Contributor

Presumably there could be a const-hack-loop and const-hack-mut-ref if anyone gets really frisky.

This comment has been minimized.

Copy link
@Centril

Centril Sep 16, 2019

Author Member

@ecstatic-morse I was wondering if we couldn't use one label for all of them?

This comment has been minimized.

Copy link
@ecstatic-morse

ecstatic-morse Sep 16, 2019

Contributor

I don't care so much. I thought it would be easier to go back and remove the labels if we know exactly which limitation is being worked around, but the absence of conditionals is probably the only thing that can be circumvented with these kinds of hacks.

This comment has been minimized.

Copy link
@Centril

Centril Sep 17, 2019

Author Member

Added a const-hack label. Please let me know if there are some PRs I missed.

This comment has been minimized.

Copy link
@oli-obk

oli-obk Sep 17, 2019

Contributor

I have a todo entry for reviewing all const fns after we get loops and conditions, I'm certain we'll miss some by whitelisting what to check

This comment has been minimized.

Copy link
@RalfJung

RalfJung Sep 17, 2019

Member

@Centril thanks!

@oli-obk Let's just go through the tag then after you are done with that review, to double-check.

@jadbox

This comment has been minimized.

Copy link

commented Sep 19, 2019

Does this improve the performance of String and Vec creation?

@Centril

This comment has been minimized.

Copy link
Member Author

commented Sep 19, 2019

I wouldn't expect it to have any perf impact one way or the other; it's just copying some code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.