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

Implement Index and IndexMut for arrays #74989

Merged
merged 1 commit into from
Nov 17, 2020

Conversation

pubfnbar
Copy link
Contributor

@pubfnbar pubfnbar commented Jul 31, 2020

Adds implementations of Index and IndexMut for arrays that simply forward to the slice indexing implementation in order to fix the following problem:

If you implement Index<MyIndexType> for an array, you lose all the other indexing functionality that used to be available to the array via its implicit coercion to a slice. An example of what I'm talking about:

use std::ops::Index;

pub enum MyIndexType {
    _0, _1, _2, _3, _4, _5, _6, _7,
}

impl<T> Index<MyIndexType> for [T; 8] {
    type Output = T;

    fn index(&self, index: MyIndexType) -> &T {
        unsafe { self.get_unchecked(index as usize) }
    }
}

fn main() {
    let array = [11u8; 8];

    println!("{:?}", array[MyIndexType::_0]); // OK
    
    println!("{:?}", array[0usize]); // error[E0277]
    //               ^^^^^^^^^^^^^ `[u8; 8]` cannot be indexed by `usize`
}

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @hanna-kruppe (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 31, 2020
@scottmcm scottmcm added the needs-fcp This change is insta-stable, so needs a completed FCP to proceed. label Jul 31, 2020
@scottmcm

This comment has been minimized.

@scottmcm scottmcm added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jul 31, 2020
@pubfnbar
Copy link
Contributor Author

pubfnbar commented Aug 1, 2020

Just to speculate a bit... If it becomes possible to use generic parameters in evaluating constant expressions, and if a BoundedUsize type (that wraps a bounded usize value) is added to the standard library and we want to be able to use it to index (large enough) arrays without bounds checking, it might look something like this:

pub struct Predicate<const P: bool>;

pub trait Satisfied {}

impl Satisfied for Predicate<true> {}

// Guarantees that if `x` is a `BoundedUsize<A, B>` value, then `x.0 >= A && x.0 <= B && A <= B`
pub struct BoundedUsize<const A: usize, const B: usize>(usize)
where Predicate<{ A <= B }>: Satisfied;

impl<T, const N: usize, const A: usize, const B: usize> Index<BoundedUsize<A, B>> for [T; N]
where Predicate<{ A <= B && B < N }>: Satisfied
{
    type Output = T;

    fn index(&self, index: BoundedUsize<A, B>) -> &T {
        unsafe { self.get_unchecked(index.0) }
    }
}

@scottmcm
Copy link
Member

scottmcm commented Aug 2, 2020

Note that, when LLVM knows the index is in-bounds, it already removes the bounds check, even when indirected through a slice.
Demo: https://rust.godbolt.org/z/hWYG58

@pubfnbar
Copy link
Contributor Author

pubfnbar commented Aug 2, 2020

It would be interesting to test whether this PR breaks any code in crates.io (but I don't know how to do that). I'm pretty sure that at least the following code would stop compiling:

use std::ops::Index;

enum Idx {
    _0,
    _1,
}

impl<T> Index<Idx> for [T] {
    type Output = T;

    fn index(&self, idx: Idx) -> &T {
        Index::index(self, idx as usize)
    }
}

// This PR would cause there to be a clashing std library implementation of `Index<Idx> for [T; 2]`
impl<T> Index<Idx> for [T; 2] {
    type Output = T;

    fn index(&self, idx: Idx) -> &T {
        unsafe { self.get_unchecked(idx as usize) }
    }
}

fn main() {
    let array = [11, 22];
    let slice: &[u8] = &array;
    
    println!("{:?}", array[Idx::_0]);
    println!("{:?}", slice[Idx::_0]);
}

I don't know if this means that this PR can be merged only in the next Rust Edition?

@Dylan-DPC-zz
Copy link

r? @KodrAus

@pubfnbar
Copy link
Contributor Author

I'd like to clarify something. You might be wondering how it is even possible (for some non-std code) to implement an external trait (Index) for an external type (array) given that even The Book says that the orphan rule dictates that: "But we can’t implement external traits on external types". The answer is that this rule was relaxed by RFC #2451 - Re-Rebalancing Coherence, and you can read more about it here:

@KodrAus
Copy link
Contributor

KodrAus commented Sep 4, 2020

Based on the precedent of #74060 I'd be comfortable stabilizing these signatures with their use of const generics, and since this has only been possible to write for ~6months I wouldn't expect a lot of breakage. But just in case anybody else thinks a crater run is necessary I'll kick one off that we can cancel if we don't think it's needed.

@rfcbot fcp merge

This proposes stabilizing implementations of Index for [T; N] that forward to [T].

@rfcbot
Copy link

rfcbot commented Sep 4, 2020

Team member @KodrAus 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 rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Sep 4, 2020
@KodrAus
Copy link
Contributor

KodrAus commented Sep 4, 2020

@bors try

@bors
Copy link
Contributor

bors commented Sep 4, 2020

⌛ Trying commit ef61d686043f1df450d5a580978b76af985da6d6 with merge 72f88fc4e9458f89516a0b8b0868f3718aec5b47...

@bors
Copy link
Contributor

bors commented Sep 4, 2020

☀️ Try build successful - checks-actions, checks-azure
Build commit: 72f88fc4e9458f89516a0b8b0868f3718aec5b47 (72f88fc4e9458f89516a0b8b0868f3718aec5b47)

@KodrAus
Copy link
Contributor

KodrAus commented Sep 4, 2020

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-74989 created and queued.
🤖 Automatically detected try build 72f88fc4e9458f89516a0b8b0868f3718aec5b47
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 4, 2020
@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Sep 4, 2020
@rfcbot
Copy link

rfcbot commented Sep 4, 2020

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

@spastorino spastorino removed the to-announce Announce this issue on triage meeting label Sep 16, 2020
@KodrAus
Copy link
Contributor

KodrAus commented Sep 22, 2020

It looks like we have a handful of regressions here for 2d indexes:

Before we merge I'll follow up with these libraries (it looks like kit is the only one up on crates.io right now)

@crlf0710 crlf0710 added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 8, 2020
@crlf0710 crlf0710 added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Oct 30, 2020
@pubfnbar
Copy link
Contributor Author

Just wondering, why hasn't this been merged already?

Copy link
Contributor

@KodrAus KodrAus 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 ping @pubfnbar!

It looks like I didn't pay close enough attention to the breakage in the published crate here. It's actually unrelated.

I've left some edits to bump stabilization to 1.49.0. After that we should be good to merge.

library/core/src/array/mod.rs Outdated Show resolved Hide resolved
library/core/src/array/mod.rs Outdated Show resolved Hide resolved
@Mark-Simulacrum
Copy link
Member

Please squash commits into one, too.

@pubfnbar
Copy link
Contributor Author

Please squash commits into one, too.

@Mark-Simulacrum
If this was directed to me, then I must admit I don't know how to do that. After some googling I think it means I should download (or whatever the correct terminology is) this pull request (or maybe the whole Rust repository?) to my computer and use a command like git rebase <something>. I don't really know how to use git or GitHub - I created this pull request only by using this website (I've never done the git download thingy to my computer). But it seems that whoever is allowed to merge this can also "squash" the commits while he's at it (according to this).

Adds implementations of `Index` and `IndexMut` for arrays that simply forward to the slice indexing implementation.
@Mark-Simulacrum
Copy link
Member

No worries, squashed for you (also bumped to 1.50, as that's the current nightly version).

@bors r=KodrAus rollup

https://rustc-dev-guide.rust-lang.org/git.html has some documentation on getting started with git, if you're interested, and feel free to drop by Zulip with any questions!

@bors
Copy link
Contributor

bors commented Nov 16, 2020

📌 Commit c03dfa6 has been approved by KodrAus

@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-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Nov 16, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 16, 2020
Rollup of 11 pull requests

Successful merges:

 - rust-lang#74989 (Implement `Index` and `IndexMut` for arrays)
 - rust-lang#76339 (Test structural matching for all range types)
 - rust-lang#77691 (Rename/Deprecate LayoutErr in favor of LayoutError)
 - rust-lang#78364 (Update RELEASES.md for 1.48.0)
 - rust-lang#78678 (Add tests and improve rendering of cfgs on traits)
 - rust-lang#78714 (Simplify output capturing)
 - rust-lang#78769 (Remove unneeded lifetimes in array/mod.rs)
 - rust-lang#78903 (BTreeMap: test chaotic ordering & other bits & bobs)
 - rust-lang#79032 (improve type const mismatch errors)
 - rust-lang#79061 (Make all rustdoc functions and structs crate-private)
 - rust-lang#79087 (Update E0744 about control flow in `const` contexts to accurately describe when the error is triggered and why)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 4cdd220 into rust-lang:master Nov 17, 2020
@rustbot rustbot added this to the 1.50.0 milestone Nov 17, 2020
@pubfnbar pubfnbar deleted the impl-array-indexing branch November 17, 2020 01:18
@lcnr lcnr added the F-const_generics `#![feature(const_generics)]` label Nov 17, 2020
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Feb 14, 2021
Pkgsrc changes:
 * Adjust patches, convert tabs to spaces so that tests pass.
 * Remove patches which are no longer needed (upstream changed)
 * Minor adjustments for SunOS, e.g. disable stack probes.
 * Adjust cargo checksum patching accordingly.
 * Remove commented-out use of PATCHELF on NetBSD, which doesn't work anyway...

Upstream changes:

Version 1.49.0 (2020-12-31)
============================

Language
-----------------------

- [Unions can now implement `Drop`, and you can now have a field in a union
  with `ManuallyDrop<T>`.][77547]
- [You can now cast uninhabited enums to integers.][76199]
- [You can now bind by reference and by move in patterns.][76119] This
  allows you to selectively borrow individual components of a type. E.g.
  ```rust
  #[derive(Debug)]
  struct Person {
      name: String,
      age: u8,
  }

  let person = Person {
      name: String::from("Alice"),
      age: 20,
  };

  // `name` is moved out of person, but `age` is referenced.
  let Person { name, ref age } = person;
  println!("{} {}", name, age);
  ```

Compiler
-----------------------

- [Added tier 1\* support for `aarch64-unknown-linux-gnu`.][78228]
- [Added tier 2 support for `aarch64-apple-darwin`.][75991]
- [Added tier 2 support for `aarch64-pc-windows-msvc`.][75914]
- [Added tier 3 support for `mipsel-unknown-none`.][78676]
- [Raised the minimum supported LLVM version to LLVM 9.][78848]
- [Output from threads spawned in tests is now captured.][78227]
- [Change os and vendor values to "none" and "unknown" for some targets][78951]

\* Refer to Rust's [platform support page][forge-platform-support] for more
information on Rust's tiered platform support.

Libraries
-----------------------

- [`RangeInclusive` now checks for exhaustion when calling `contains`
  and indexing.][78109]
- [`ToString::to_string` now no longer shrinks the internal buffer
  in the default implementation.][77997]
- [`ops::{Index, IndexMut}` are now implemented for fixed sized
  arrays of any length.][74989]

Stabilized APIs
---------------

- [`slice::select_nth_unstable`]
- [`slice::select_nth_unstable_by`]
- [`slice::select_nth_unstable_by_key`]

The following previously stable methods are now `const`.

- [`Poll::is_ready`]
- [`Poll::is_pending`]

Cargo
-----------------------
- [Building a crate with `cargo-package` should now be independently
  reproducible.][cargo/8864]
- [`cargo-tree` now marks proc-macro crates.][cargo/8765]
- [Added `CARGO_PRIMARY_PACKAGE` build-time environment
  variable.]  [cargo/8758] This variable will be set if the crate
  being built is one the user selected to build, either with `-p`
  or through defaults.
- [You can now use glob patterns when specifying packages &
  targets.][cargo/8752]


Compatibility Notes
-------------------
- [Demoted `i686-unknown-freebsd` from host tier 2 to target tier
  2 support.][78746]
- [Macros that end with a semi-colon are now treated as statements
  even if they expand to nothing.][78376]
- [Rustc will now check for the validity of some built-in attributes
  on enum variants.][77015] Previously such invalid or unused
  attributes could be ignored.
- Leading whitespace is stripped more uniformly in documentation
  comments, which may change behavior. You read [this post about
  the changes][rustdoc-ws-post] for more details.
- [Trait bounds are no longer inferred for associated types.][79904]

Internal Only
-------------
These changes provide no direct user facing benefits, but represent
significant improvements to the internals and overall performance
of rustc and related tools.

- [rustc's internal crates are now compiled using the `initial-exec` Thread
  Local Storage model.][78201]
- [Calculate visibilities once in resolve.][78077]
- [Added `system` to the `llvm-libunwind` bootstrap config option.][77703]
- [Added `--color` for configuring terminal color support to bootstrap.][79004]


[75991]: rust-lang/rust#75991
[78951]: rust-lang/rust#78951
[78848]: rust-lang/rust#78848
[78746]: rust-lang/rust#78746
[78376]: rust-lang/rust#78376
[78228]: rust-lang/rust#78228
[78227]: rust-lang/rust#78227
[78201]: rust-lang/rust#78201
[78109]: rust-lang/rust#78109
[78077]: rust-lang/rust#78077
[77997]: rust-lang/rust#77997
[77703]: rust-lang/rust#77703
[77547]: rust-lang/rust#77547
[77015]: rust-lang/rust#77015
[76199]: rust-lang/rust#76199
[76119]: rust-lang/rust#76119
[75914]: rust-lang/rust#75914
[74989]: rust-lang/rust#74989
[79004]: rust-lang/rust#79004
[78676]: rust-lang/rust#78676
[79904]: rust-lang/rust#79904
[cargo/8864]: rust-lang/cargo#8864
[cargo/8765]: rust-lang/cargo#8765
[cargo/8758]: rust-lang/cargo#8758
[cargo/8752]: rust-lang/cargo#8752
[`slice::select_nth_unstable`]: https://doc.rust-lang.org/nightly/std/primitive.slice.html#method.select_nth_unstable
[`slice::select_nth_unstable_by`]: https://doc.rust-lang.org/nightly/std/primitive.slice.html#method.select_nth_unstable_by
[`slice::select_nth_unstable_by_key`]: https://doc.rust-lang.org/nightly/std/primitive.slice.html#method.select_nth_unstable_by_key
[`hint::spin_loop`]: https://doc.rust-lang.org/stable/std/hint/fn.spin_loop.html
[`Poll::is_ready`]: https://doc.rust-lang.org/stable/std/task/enum.Poll.html#method.is_ready
[`Poll::is_pending`]: https://doc.rust-lang.org/stable/std/task/enum.Poll.html#method.is_pending
[rustdoc-ws-post]: https://blog.guillaume-gomez.fr/articles/2020-11-11+New+doc+comment+handling+in+rustdoc
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Apr 19, 2021
Pkgsrc changes:
 * Adjust patches, re-compute line offsets, fix capitalization.
 * Remove i686/FreeBSD support, no longer provided upstream.
 * Bump bootstraps to 1.49.0.
 * Change USE_TOOLS from bsdtar to gtar.
 * Reduce diffs to pkgsrc-wip package patches.
 * Allow rust.BUILD_TARGET to override automatic choice of target.
 * Add an i586/NetBSD (pentium) bootstrap variant (needs testing),
   not yet added as bootstrap since 1.49 doesn't have that variant.

Upstream changes:

Version 1.50.0 (2021-02-11)
============================

Language
-----------------------
- [You can now use `const` values for `x` in `[x; N]` array
  expressions.][79270] This has been technically possible since
  1.38.0, as it was unintentionally stabilized.
- [Assignments to `ManuallyDrop<T>` union fields are now considered
  safe.][78068]

Compiler
-----------------------
- [Added tier 3\* support for the `armv5te-unknown-linux-uclibceabi`
  target.][78142]
- [Added tier 3 support for the `aarch64-apple-ios-macabi` target.][77484]
- [The `x86_64-unknown-freebsd` is now built with the full toolset.][79484]

\* Refer to Rust's [platform support page][forge-platform-support] for more
information on Rust's tiered platform support.

Libraries
-----------------------

- [`proc_macro::Punct` now implements `PartialEq<char>`.][78636]
- [`ops::{Index, IndexMut}` are now implemented for fixed sized
  arrays of any length.][74989]
- [On Unix platforms, the `std::fs::File` type now has a "niche"
  of `-1`.][74699] This value cannot be a valid file descriptor,
  and now means `Option<File>` takes up the same amount of space
  as `File`.

Stabilized APIs
---------------

- [`bool::then`]
- [`btree_map::Entry::or_insert_with_key`]
- [`f32::clamp`]
- [`f64::clamp`]
- [`hash_map::Entry::or_insert_with_key`]
- [`Ord::clamp`]
- [`RefCell::take`]
- [`slice::fill`]
- [`UnsafeCell::get_mut`]

The following previously stable methods are now `const`.

- [`IpAddr::is_ipv4`]
- [`IpAddr::is_ipv6`]
- [`Layout::size`]
- [`Layout::align`]
- [`Layout::from_size_align`]
- `pow` for all integer types.
- `checked_pow` for all integer types.
- `saturating_pow` for all integer types.
- `wrapping_pow` for all integer types.
- `next_power_of_two` for all unsigned integer types.
- `checked_power_of_two` for all unsigned integer types.

Cargo
-----------------------

- [Added the `[build.rustc-workspace-wrapper]` option.][cargo/8976]
  This option sets a wrapper to execute instead of `rustc`, for
  workspace members only.
- [`cargo:rerun-if-changed` will now, if provided a directory, scan the entire
  contents of that directory for changes.][cargo/8973]
- [Added the `--workspace` flag to the `cargo update` command.][cargo/8725]

Misc
----

- [The search results tab and the help button are focusable with
  keyboard in rustdoc.][79896]
- [Running tests will now print the total time taken to execute.][75752]

Compatibility Notes
-------------------

- [The `compare_and_swap` method on atomics has been deprecated.][79261]
  It's recommended to use the `compare_exchange` and
  `compare_exchange_weak` methods instead.
- [Changes in how `TokenStream`s are checked have fixed some cases
  where you could write unhygenic `macro_rules!` macros.][79472]
- [`#![test]` as an inner attribute is now considered unstable like
  other inner macro attributes, and reports an error by default
  through the `soft_unstable` lint.][79003]
- [Overriding a `forbid` lint at the same level that it was set is
  now a hard error.][78864]
- [Dropped support for all cloudabi targets.][78439]
- [You can no longer intercept `panic!` calls by supplying your
  own macro.][78343] It's recommended to use the `#[panic_handler]`
  attribute to provide your own implementation.
- [Semi-colons after item statements (e.g. `struct Foo {};`) now
  produce a warning.][78296]

[74989]: rust-lang/rust#74989
[79261]: rust-lang/rust#79261
[79896]: rust-lang/rust#79896
[79484]: rust-lang/rust#79484
[79472]: rust-lang/rust#79472
[79270]: rust-lang/rust#79270
[79003]: rust-lang/rust#79003
[78864]: rust-lang/rust#78864
[78636]: rust-lang/rust#78636
[78439]: rust-lang/rust#78439
[78343]: rust-lang/rust#78343
[78296]: rust-lang/rust#78296
[78068]: rust-lang/rust#78068
[75752]: rust-lang/rust#75752
[74699]: rust-lang/rust#74699
[78142]: rust-lang/rust#78142
[77484]: rust-lang/rust#77484
[cargo/8976]: rust-lang/cargo#8976
[cargo/8973]: rust-lang/cargo#8973
[cargo/8725]: rust-lang/cargo#8725
[`IpAddr::is_ipv4`]: https://doc.rust-lang.org/stable/std/net/enum.IpAddr.html#method.is_ipv4
[`IpAddr::is_ipv6`]: https://doc.rust-lang.org/stable/std/net/enum.IpAddr.html#method.is_ipv6
[`Layout::align`]: https://doc.rust-lang.org/stable/std/alloc/struct.Layout.html#method.align
[`Layout::from_size_align`]: https://doc.rust-lang.org/stable/std/alloc/struct.Layout.html#method.from_size_align
[`Layout::size`]: https://doc.rust-lang.org/stable/std/alloc/struct.Layout.html#method.size
[`Ord::clamp`]: https://doc.rust-lang.org/stable/std/cmp/trait.Ord.html#method.clamp
[`RefCell::take`]: https://doc.rust-lang.org/stable/std/cell/struct.RefCell.html#method.take
[`UnsafeCell::get_mut`]: https://doc.rust-lang.org/stable/std/cell/struct.UnsafeCell.html#method.get_mut
[`bool::then`]: https://doc.rust-lang.org/stable/std/primitive.bool.html#method.then
[`btree_map::Entry::or_insert_with_key`]: https://doc.rust-lang.org/stable/std/collections/btree_map/enum.Entry.html#method.or_insert_with_key
[`f32::clamp`]: https://doc.rust-lang.org/stable/std/primitive.f32.html#method.clamp
[`f64::clamp`]: https://doc.rust-lang.org/stable/std/primitive.f64.html#method.clamp
[`hash_map::Entry::or_insert_with_key`]: https://doc.rust-lang.org/stable/std/collections/hash_map/enum.Entry.html#method.or_insert_with_key
[`slice::fill`]: https://doc.rust-lang.org/stable/std/primitive.slice.html#method.fill
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. F-const_generics `#![feature(const_generics)]` finished-final-comment-period The final comment period is finished for this PR / Issue. needs-fcp This change is insta-stable, so needs a completed FCP to proceed. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.