Skip to content

Use alloc for no_std builds#924

Merged
TheBlueMatt merged 1 commit intolightningdevkit:mainfrom
GeneFerneau:alloc
May 27, 2021
Merged

Use alloc for no_std builds#924
TheBlueMatt merged 1 commit intolightningdevkit:mainfrom
GeneFerneau:alloc

Conversation

@GeneFerneau
Copy link
Copy Markdown
Contributor

@GeneFerneau GeneFerneau commented May 19, 2021

Replace std structs with alloc equivalents to support no_std builds

@GeneFerneau GeneFerneau mentioned this pull request May 19, 2021
Closed
devrandom
devrandom reviewed May 19, 2021
View reviewed changes
@devrandom
Copy link
Copy Markdown
Member

devrandom commented May 19, 2021

Concept ACK

@TheBlueMatt
Copy link
Copy Markdown
Collaborator

TheBlueMatt commented May 19, 2021

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

@devrandom
Copy link
Copy Markdown
Member

devrandom commented May 19, 2021

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

@codecov
Copy link
Copy Markdown

codecov bot commented May 19, 2021 •
edited
Loading

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
Copy Markdown
Collaborator

TheBlueMatt commented May 19, 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.

@GeneFerneau
Copy link
Copy Markdown
Contributor Author

GeneFerneau commented May 22, 2021 •
edited
Loading

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
Copy Markdown
Member

devrandom commented May 24, 2021 •
edited
Loading

The following are missing imports of Vec:

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

@devrandom
Copy link
Copy Markdown
Member

devrandom commented May 24, 2021

What about String?

TheBlueMatt
TheBlueMatt reviewed May 24, 2021
View reviewed changes
@GeneFerneau GeneFerneau force-pushed the alloc branch 2 times, most recently from fb308a1 to dfdded0 Compare May 26, 2021 19:09
@GeneFerneau
Copy link
Copy Markdown
Contributor Author

GeneFerneau commented May 26, 2021

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

devrandom
devrandom approved these changes May 27, 2021
View reviewed changes
Copy link
Copy Markdown
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

TheBlueMatt
TheBlueMatt approved these changes May 27, 2021
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

GeneFerneau commented May 27, 2021

LGTM. Can you squash the fixup commit down?

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

@GeneFerneau
Use alloc for no_std builds
12461fc 
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

Reviewers

@devrandom devrandom devrandom approved these changes

@TheBlueMatt TheBlueMatt TheBlueMatt approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@GeneFerneau @devrandom @TheBlueMatt