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

Use alloc for no_std builds #924

Merged
merged 1 commit into from
May 27, 2021
Merged

Conversation

GeneFerneau
Copy link
Contributor

Replace std structs with alloc equivalents to support no_std builds

@devrandom
Copy link
Member

Concept ACK

@TheBlueMatt
Copy link
Collaborator

Shouldn't this be dropping every use std... statement? Or are we allowed to use std... on no_std builds?

@devrandom
Copy link
Member

If I understand correctly, this is a smaller step towards no_std.

@codecov
Copy link

codecov bot commented May 19, 2021

Codecov Report

Merging #924 (735f96c) into main (f074343) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 735f96c differs from pull request most recent head 12461fc. Consider uploading reports for the commit 12461fc to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##             main     #924   +/-   ##
=======================================
  Coverage   90.47%   90.47%           
=======================================
  Files          60       60           
  Lines       30572    30572           
=======================================
  Hits        27660    27660           
  Misses       2912     2912           
Impacted Files Coverage Δ
lightning/src/chain/channelmonitor.rs 91.03% <ø> (ø)
lightning/src/chain/keysinterface.rs 94.96% <ø> (ø)
lightning/src/chain/mod.rs 100.00% <ø> (ø)
lightning/src/chain/onchaintx.rs 93.68% <ø> (ø)
lightning/src/chain/package.rs 92.32% <ø> (ø)
lightning/src/ln/chan_utils.rs 97.34% <ø> (ø)
lightning/src/ln/chanmon_update_fail_tests.rs 97.77% <ø> (ø)
lightning/src/ln/channel.rs 88.24% <ø> (ø)
lightning/src/ln/features.rs 98.83% <ø> (ø)
lightning/src/ln/functional_test_utils.rs 94.90% <ø> (ø)
... and 24 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f074343...12461fc. Read the comment docs.

@TheBlueMatt
Copy link
Collaborator

Right, we could take this separately, I suppose. I just think the big std -> core "rename" is half of the work on landing a no_std version, so good to do that in a separate PR as well.

@GeneFerneau
Copy link
Contributor Author

GeneFerneau commented May 22, 2021

Right, we could take this separately, I suppose. I just think the big std -> core "rename" is half of the work on landing a no_std version, so good to do that in a separate PR as well.

So, I split these changes per your request in #842. My thought in not enabling #![no_std], and not doing a full rename was to incrementally introduce the changes needed to enable a no_std build.

The next PR I will make will include the std -> core "rename", then another separate PR replacing HashMap and friends with hashbrown equivalents. Another for the literacy::* replacements for std::io, and finally one for the remaining std::sync stuff that relies on OS abstractions.

Basically, I am breaking up #842 into small, digestible chunks to make review easier. This should hopefully allow for an inching toward a no_std build, instead of an all-or-nothing drawn out thing in one PR.

@devrandom
Copy link
Member

devrandom commented May 24, 2021

The following are missing imports of Vec:

  • messaging_signing.rs
  • ln/mod.rs
  • zbase32.rs

@devrandom
Copy link
Member

What about String?

lightning/src/lib.rs Outdated Show resolved Hide resolved
@GeneFerneau GeneFerneau force-pushed the alloc branch 2 times, most recently from fb308a1 to dfdded0 Compare May 26, 2021 19:09
@GeneFerneau
Copy link
Contributor Author

Rebased on latest main, changes now include some std -> core renames.

Copy link
Member

@devrandom devrandom left a comment

Choose a reason for hiding this comment

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

ACK

Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

LGTM. Can you squash the fixup commit down?

@GeneFerneau
Copy link
Contributor Author

LGTM. Can you squash the fixup commit down?

Yeah, I just wanted to give credit to @devrandom for the fix

Replace std structs with alloc equivalents to support no_std builds

f use prelude::* credit @devrandom
@TheBlueMatt TheBlueMatt merged commit f30694b into lightningdevkit:main May 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants