Skip to content

Should we include commented-out libcore in avr-rust? #54

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

Closed
dylanmckay opened this issue May 13, 2017 · 9 comments
Closed

Should we include commented-out libcore in avr-rust? #54

dylanmckay opened this issue May 13, 2017 · 9 comments
Labels
A-libcore Affects compiling the core library A-rust Affects overall Rust compiler architecture question

Comments

@dylanmckay
Copy link
Member

I was wondering what your thoughts were @shepmaster and @gergoerdi on pushing one of the minified versions of libcore you both have so that it is a part of this repo?

There isn't much point in maintaining separate local libcore libraries.

One concern is that it may mean that we get more merge conflicts. We would probably have to comment out parts of libcore in this repository anyway for stuff like float/libcore support.

@gergoerdi
Copy link
Collaborator

My version is here: https://github.com/gergoerdi/rust-avr-libcore-mini I'm happy to move it wherever. It is based on libcore as of e80f644 with the following modules (and references to them) removed:

  • any.rs
  • cell
  • char
  • char_private
  • fmt::*
  • hash::*
  • num::{bignum, dec2flt, flt2dec, diy_float}
  • raw
  • slice::sort
  • str
  • sync::*
  • tuple

@shepmaster
Copy link
Member

The big trick to me is that we need the full libcore to build the compiler for the host but when targeting AVR we need the minimized libcore for now.

Commenting out wouldn't be sufficient for that, unless we actually had two directories, at which point the merge conflicts would go to zero but the updating process would get much harder ;-)

Perhaps #[cfg(not(target_arch = "avr"))] (or whatever is the real value) would be better than commenting out stuff. Would also be good markers for use to remove as we go along.

Beyond that, I'd be happy to see it present here.

@shepmaster
Copy link
Member

As a rough scope:

$ diff --unified -r libcore/ rust-avr-libcore-mini/src/ | wc
    2084    8801   68577

This includes changes like

-#[derive(Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Debug, Hash)]
+#[derive(Copy, Clone, Eq, PartialEq, Ord, PartialOrd)]

Which I'm sure was annoying to do and will be annoying to bracket in cfg blocks.

@shepmaster shepmaster added A-rust Affects overall Rust compiler architecture question labels May 13, 2017
@shepmaster
Copy link
Member

I feel like most of the Debug and formatting differences would be taken care of by #37, which is why I'd be more excited to see that fixed instead of commenting out every Debug implementation.

@gergoerdi
Copy link
Collaborator

In the meantime, couldn't we reduce the diff considerably by keeping Debug and Hash and just stubbing them out, instead of removing them? Let me try and see how it'd work.

@gergoerdi
Copy link
Collaborator

gergoerdi commented May 14, 2017

I've got great news! I've just pushed a new version of libcore-mini (overwriting master) that is almost identical to the real libcore, but still works well enough that I can compile my CHIP-8 program with it:

19:05:56 [cactus@galaxy src]$ diff -ur $CORE .|wc
    957    4306   33726

@gergoerdi
Copy link
Collaborator

The remaining changes compare to libcore are, I believe, rather benign and concentrated (no more "I had to remove Hash and Debug derivation from every. single. type.")

  • Float and i128/u128 support is removed
  • All the panicking macros just loop{}
  • The sync module is removed (I haven't investigated yet why this is needed)
  • Most of the fmt module is stubbed out
  • String <-> number conversion is removed

@gergoerdi
Copy link
Collaborator

19:22:23 [cactus@galaxy src]$ diff -qr $CORE .
Only in /home/cactus/prog/rust/rust-avr/rust/src/libcore: Cargo.toml
Only in /home/cactus/prog/rust/rust-avr/rust/src/libcore: benches
Files /home/cactus/prog/rust/rust-avr/rust/src/libcore/fmt/mod.rs and ./fmt/mod.rs differ
Files /home/cactus/prog/rust/rust-avr/rust/src/libcore/fmt/num.rs and ./fmt/num.rs differ
Files /home/cactus/prog/rust/rust-avr/rust/src/libcore/iter/range.rs and ./iter/range.rs differ
Files /home/cactus/prog/rust/rust-avr/rust/src/libcore/lib.rs and ./lib.rs differ
Files /home/cactus/prog/rust/rust-avr/rust/src/libcore/macros.rs and ./macros.rs differ
Files /home/cactus/prog/rust/rust-avr/rust/src/libcore/num/mod.rs and ./num/mod.rs differ
Files /home/cactus/prog/rust/rust-avr/rust/src/libcore/num/wrapping.rs and ./num/wrapping.rs differ
Files /home/cactus/prog/rust/rust-avr/rust/src/libcore/panicking.rs and ./panicking.rs differ
Files /home/cactus/prog/rust/rust-avr/rust/src/libcore/str/mod.rs and ./str/mod.rs differ
Only in /home/cactus/prog/rust/rust-avr/rust/src/libcore: tests

@gergoerdi
Copy link
Collaborator

More details on the magnitude of the difference from upstream libcore:

 src/fmt/mod.rs      | 310 ++--------------------------------------------------
 src/fmt/num.rs      |   6 +-
 src/iter/range.rs   |   1 -
 src/lib.rs          |   9 +-
 src/macros.rs       |   9 +-
 src/num/mod.rs      | 282 +----------------------------------------------
 src/num/wrapping.rs |   2 +-
 src/panicking.rs    |  12 +-
 src/str/mod.rs      |  27 +----
 9 files changed, 26 insertions(+), 632 deletions(-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-libcore Affects compiling the core library A-rust Affects overall Rust compiler architecture question
Projects
None yet
Development

No branches or pull requests

3 participants