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 Vec::new a const fn #50233

Merged
merged 7 commits into from
Apr 30, 2018
Merged

Make Vec::new a const fn #50233

merged 7 commits into from
Apr 30, 2018

Conversation

mark-i-m
Copy link
Member

RawVec::empty/_in are a hack. They're there because if size_of::<T> == 0 { !0 } else { 0 } is not allowed in const yet. However, because RawVec is unstable, the empty/empty_in constructors can be removed when #49146 is done...

@rust-highfive
Copy link
Collaborator

r? @kennytm

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 25, 2018
@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-3.9 of your PR failed on Travis (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.
/usr/local/lib/python2.7/dist-packages/pip/_vendor/requests/packages/urllib3/util/ssl_.py:122: InsecurePlatformWarning: A true SSLContext object is not available. This prevents urllib3 from configuring SSL appropriately and may cause certain SSL connections to fail. You can upgrade to a newer version of Python to solve this. For more information, see https://urllib3.readthedocs.io/en/latest/security.html#insecureplatformwarning.
  InsecurePlatformWarning
/usr/local/lib/python2.7/dist-packages/pip/_vendor/requests/packages/urllib3/util/ssl_.py:122: InsecurePlatformWarning: A true SSLContext object is not available. This prevents urllib3 from configuring SSL appropriately and may cause certain SSL connections to fail. You can upgrade to a newer version of Python to solve this. For more information, see https://urllib3.readthedocs.io/en/latest/security.html#insecureplatformwarning.
  InsecurePlatformWarning
  Downloading https://files.pythonhosted.org/packages/4a/7d/9c0f35dc594b78137929523f9632ec649b69af2d30ca6a7a8c60d414b23a/awscli-1.15.9-py2.py3-none-any.whl (1.3MB)
    0% |▎                               | 10kB 11.4MB/s eta 0:00:01
    1% |▌                               | 20kB 1.9MB/s eta 0:00:01
    2% |▉                               | 30kB 2.2MB/s eta 0:00:01
    3% |█                               | 40kB 1.9MB/s eta 0:00:01
---
    100% |████████████████████████████████| 61kB 8.5MB/s 
Collecting botocore==1.10.9 (from awscli)
/usr/local/lib/python2.7/dist-packages/pip/_vendor/requests/packages/urllib3/util/ssl_.py:122: InsecurePlatformWarning: A true SSLContext object is not available. This prevents urllib3 from configuring SSL appropriately and may cause certain SSL connections to fail. You can upgrade to a newer version of Python to solve this. For more information, see https://urllib3.readthedocs.io/en/latest/security.html#insecureplatformwarning.
  InsecurePlatformWarning
  Downloading https://files.pythonhosted.org/packages/fe/13/45a8eeb5d78e2578b8e55df58e3921369efe51eaa57a9d2a36e7bef45bcc/botocore-1.10.9-py2.py3-none-any.whl (4.2MB)
    0% |                                | 10kB 43.2MB/s eta 0:00:01
    0% |▏                               | 20kB 40.1MB/s eta 0:00:01
    0% |▎                               | 30kB 45.2MB/s eta 0:00:01
    0% |▎                               | 40kB 27.2MB/s eta 0:00:01
---
[00:54:20] ......i........................................................................................test [run-pass] run-pass/mir_heavy_promoted.rs has been running for over 60 seconds
[00:54:23] .....
[00:54:55] ....................................................................................................
[00:55:24] .......................................................................ii...........................
[00:56:17] ..................................i....................................................i.ii........test [run-pass] run-pass/saturating-float-casts.rs has been running for over 60 seconds
[00:56:59] ...............................................................................................iiiii
[00:57:25] ii..................................................................................................
[00:57:55] ....................................................................................................
[00:57:55] ....................................................................................................
[00:58:20] ..............................................................................................F.....
[00:58:38] failures:
[00:58:38] 
[00:58:38] ---- [run-pass] run-pass/vec-const-new.rs stdout ----
[00:58:38]  
[00:58:38]  
[00:58:38] error: compilation failed!
[00:58:38] status: exit code: 101
[00:58:38] command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/run-pass/vec-const-new.rs" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass" "--target=x86_64-unknown-linux-gnu" "-C" "prefer-dynamic" "-o" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/vec-const-new.stage2-x86_64-unknown-linux-gnu" "-Crpath" "-O" "-Zunstable-options" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/vec-const-new.stage2-x86_64-unknown-linux-gnu.aux"
[00:58:38] ------------------------------------------
[00:58:38] 
[00:58:38] ------------------------------------------
[00:58:38] stderr:
[00:58:38] stderr:
[00:58:38] ------------------------------------------
[00:58:38] error[E0015]: calls in constants are limited to constant functions, tuple structs and tuple variants
[00:58:38]    |
[00:58:38]    |
[00:58:38] 13 | const MY_VEC: Vec<usize> = Vec::new();
[00:58:38] 
[00:58:38] error: aborting due to previous error
[00:58:38] 
[00:58:38] For more information about this error, try `rustc --explain E0015`.
---
[00:58:38] 
[00:58:38] thread 'main' panicked at 'Some tests failed', tools/compiletest/src/main.rs:488:22
[00:58:38] 
[00:58:38] 
[00:58:38] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/compiletest" "--compile-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib" "--run-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib" "--rustc-path" "/checkout/obj/build/x86_64-unknown-linux-gnuWed, 25 Apr 2018 22:43:06 GMT

The command "date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
" exited with 0.
travis_fold:start:after_failure.1

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)

@@ -324,7 +324,7 @@ impl<T> Vec<T> {
#[stable(feature = "rust1", since = "1.0.0")]
pub fn new() -> Vec<T> {
Copy link
Contributor

Choose a reason for hiding this comment

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

You forgot the actual change of this PR 😆

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh. 🤦‍♂️ Thanks!

@@ -322,9 +322,9 @@ impl<T> Vec<T> {
/// ```
#[inline]
#[stable(feature = "rust1", since = "1.0.0")]
pub fn new() -> Vec<T> {
pub const fn new() -> Vec<T> {
Copy link
Member

Choose a reason for hiding this comment

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

Please mark this with #[rustc_const_unstable(feature = "const_vec_new")] so we don't need to insta-stable this.

Copy link
Member Author

Choose a reason for hiding this comment

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

@kennytm Done.

@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-3.9 of your PR failed on Travis (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.
[00:59:27] .......i....................................................................................test [run-pass] run-pass/mir_heavy_promoted.rs has been running for over 60 seconds
[00:59:31] ........
[01:00:05] ....................................................................................................
[01:00:34] ........................................................................ii..........................
[01:01:28] ....................................i....................................................i.ii.test [run-pass] run-pass/saturating-float-casts.rs has been running for over 60 seconds
[01:02:16] .................................................................................................iii
[01:02:43] iiii................................................................................................
[01:03:14] ....................................................................................................
[01:03:39] ................................................................................................F...

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)

@oli-obk
Copy link
Contributor

oli-obk commented Apr 27, 2018

The test needs #![feature(const_vec_new)]

@mbrubeck
Copy link
Contributor

This changes the behavior for zero-sized types. Before this change, Vec::<()>::new() returns a vector with capacity usize::MAX. After this change, it returns a vector with capacity zero. I'm not sure how much we care about preserving the old behavior, but it seems worth noting explicitly.

If we want to preserve the current optimization for zero-sized types, then instead of getting rid of the if we could replace it with a const_if! macro that expands to something like [0, !0][(size_of::<T>() == 0) as usize].

@alexreg
Copy link
Contributor

alexreg commented Apr 27, 2018

if statements are coming to const-land very soon, it's worth noting!

@mark-i-m
Copy link
Member Author

@alexreg would it be possible to get an ETA on that? What does "very soon" mean exactly? If it will be in the next few weeks or so, then I think it would be worth it to wait for that. Otherwise, we could use the const_if! approach temporarily.

@mark-i-m
Copy link
Member Author

@mbrubeck That approach doesn't quite work:

error[E0401]: can't use type parameters from outer function
  --> liballoc/raw_vec.rs:63:52
   |
56 | impl<T, A: Alloc> RawVec<T, A> {
   |      - type variable from outer function
...
59 |     pub const fn new_in(a: A) -> Self {
   |                  ------ try adding a local type parameter in this method instead
...
63 |         const CAP: usize = [0, !0][(mem::size_of::<T>() == 0) as usize];
   |                                                    ^ use of type variable from outer function

@alexreg
Copy link
Contributor

alexreg commented Apr 29, 2018

@mark-i-m Can't given guarantees to be honest, since this is work that @eddyb is taking over from a branch of mine. But without putting pressure on him, a few weeks sounds feasible. I'll leave it up to you if you just want to use const_if! for now. :-)

@oli-obk
Copy link
Contributor

oli-obk commented Apr 29, 2018

If you don't use an intermediate const, then the trick should work. Const if is still ways off. So use the array trick

@alexreg
Copy link
Contributor

alexreg commented Apr 29, 2018

@oli-obk Not really. It's on @eddyb's to-do list, after a couple more small PRs. We've discussed it recently. I've already laid the groundwork for it.

@oli-obk
Copy link
Contributor

oli-obk commented Apr 29, 2018

Still. the trick works now. No need to block this PR

@alexreg
Copy link
Contributor

alexreg commented Apr 29, 2018

Yeah, I don't dispute that. ;-)

@mark-i-m
Copy link
Member Author

Hmm... tidy is segfaulting on my machine, so I can't run tests at the moment...

@mark-i-m mark-i-m changed the title [WIP] Make Vec::new a const fn Make Vec::new a const fn Apr 29, 2018
@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-3.9 of your PR failed on Travis (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.
travis_time:start:tidy
tidy check
[00:04:42] 
[00:04:42] 
[00:04:42] 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" "--quiet"
[00:04:42] expected success, got: signal: 11
[00:04:42] 
[00:04:42] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:04:42] Build completed unsuccessfully in 0:01:52
[00:04:42] Build completed unsuccessfully in 0:01:52
[00:04:42] Makefile:79: recipe for target 'tidy' failed
[00:04:42] make: *** [tidy] Error 1

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:1e03c84c
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)

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)

@mark-i-m
Copy link
Member Author

Oh 🤦‍♂️

@mark-i-m
Copy link
Member Author

@kennytm I think this is ready for another review

@kennytm
Copy link
Member

kennytm commented Apr 30, 2018

@bors r+

@bors
Copy link
Contributor

bors commented Apr 30, 2018

📌 Commit f9f9923 has been approved by kennytm

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 30, 2018
kennytm added a commit to kennytm/rust that referenced this pull request Apr 30, 2018
Make `Vec::new` a `const fn`

`RawVec::empty/_in` are a hack. They're there because `if size_of::<T> == 0 { !0 } else { 0 }` is not allowed in `const` yet. However, because `RawVec` is unstable, the `empty/empty_in` constructors can be removed when rust-lang#49146 is done...
bors added a commit that referenced this pull request Apr 30, 2018
Rollup of 7 pull requests

Successful merges:

 - #50233 (Make `Vec::new` a `const fn`)
 - #50312 (Add more links in panic docs)
 - #50316 (Fix some broken links in docs.)
 - #50325 (Add a few more tests for proc macro feature gating)
 - #50327 (Display correct unused field suggestion for nested struct patterns)
 - #50330 (check that #[used] is used only on statics)
 - #50344 (Update Cargo to 2018-04-28 122fd5be5201913d42e219e132d6569493583bca)

Failed merges:
@bors bors merged commit f9f9923 into rust-lang:master Apr 30, 2018
@F001
Copy link
Contributor

F001 commented May 5, 2018

impl String {
    pub fn new() -> String {
        String { vec: Vec::new() }
    }
}

Given that String::new() is based on Vec::new(), I think it is reasonable to make it a const fn correspondingly.
If that is the case, I'd be glad to create a pull request.

@oli-obk
Copy link
Contributor

oli-obk commented May 5, 2018

Good idea!

@F001 F001 mentioned this pull request May 5, 2018
kennytm added a commit to kennytm/rust that referenced this pull request May 9, 2018
Make `String::new()` const

Following the steps of rust-lang#50233 , make `String::new()` a `const fn`.
@mark-i-m mark-i-m deleted the const_vec branch November 14, 2018 16:40
KamilaBorowska added a commit to KamilaBorowska/rust that referenced this pull request Dec 11, 2018
Initially, rust-lang#50233 accidentally changed the capacity of empty ZST. This
was pointed out during code review. This commit adds a test to prevent
capacity of ZST vectors from accidentally changing to prevent that
from happening again.
pietroalbini added a commit to pietroalbini/rust that referenced this pull request Dec 12, 2018
Test capacity of ZST vector

Initially, rust-lang#50233 accidentally changed the capacity of empty ZST. This was pointed out during code review. This commit adds a test to prevent capacity of ZST vectors from accidentally changing to prevent that from happening again.
pietroalbini added a commit to pietroalbini/rust that referenced this pull request Dec 13, 2018
Test capacity of ZST vector

Initially, rust-lang#50233 accidentally changed the capacity of empty ZST. This was pointed out during code review. This commit adds a test to prevent capacity of ZST vectors from accidentally changing to prevent that from happening again.
kennytm added a commit to kennytm/rust that referenced this pull request Dec 13, 2018
Test capacity of ZST vector

Initially, rust-lang#50233 accidentally changed the capacity of empty ZST. This was pointed out during code review. This commit adds a test to prevent capacity of ZST vectors from accidentally changing to prevent that from happening again.
kennytm added a commit to kennytm/rust that referenced this pull request Dec 14, 2018
Test capacity of ZST vector

Initially, rust-lang#50233 accidentally changed the capacity of empty ZST. This was pointed out during code review. This commit adds a test to prevent capacity of ZST vectors from accidentally changing to prevent that from happening again.
kennytm added a commit to kennytm/rust that referenced this pull request Dec 14, 2018
Test capacity of ZST vector

Initially, rust-lang#50233 accidentally changed the capacity of empty ZST. This was pointed out during code review. This commit adds a test to prevent capacity of ZST vectors from accidentally changing to prevent that from happening again.
pietroalbini added a commit to pietroalbini/rust that referenced this pull request Dec 14, 2018
Test capacity of ZST vector

Initially, rust-lang#50233 accidentally changed the capacity of empty ZST. This was pointed out during code review. This commit adds a test to prevent capacity of ZST vectors from accidentally changing to prevent that from happening again.
pietroalbini added a commit to pietroalbini/rust that referenced this pull request Dec 15, 2018
Test capacity of ZST vector

Initially, rust-lang#50233 accidentally changed the capacity of empty ZST. This was pointed out during code review. This commit adds a test to prevent capacity of ZST vectors from accidentally changing to prevent that from happening again.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants