Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add
#![no_std]
support #64Add
#![no_std]
support #64Changes from 45 commits
6f1c9bb
4608d1f
74a4619
0dd5a74
69c57aa
bbfd0db
8fccb55
722ee54
d17f046
66d69b0
883e531
5c7ccc7
3dde835
6177b46
ba16b92
543cc55
7cb6aa0
dba717d
3c17916
6b61cd0
56a1769
38b5a73
9ee8b87
6b41645
01aa8c9
73baaef
bd61866
beb6aa0
4f62421
91ae219
79a3fae
6669ed8
f50b9c6
8c8cd68
843261c
9903961
9203271
30c62ff
4576351
e1a5f26
84d4453
6bbe8de
a1dade8
1c57ff4
4173e3f
7c67328
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests always run with std imported anyway, because the test framework itself uses std, so I don't think we need the changes in this file.
Is there a reason using alloc explicitly here benefits?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are right, I have removed it.
But I am curious if we should add a test to see if it works properly under no std? It seems that there are some testing frameworks that can achieve this: https://github.com/knurling-rs/defmt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could add a crate to the repo that imports strkey and gets built as part of testing, that provides it's own panic handler, because the only way it'll build is if std isn't imported because there can't be two panic handler and std provides one.
https://doc.rust-lang.org/nomicon/panic-handler.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the same note that's a lot of test machinery for checking something that is unlikely to be the case since the no_std attribute is in use.
The risk is that it depends on a crate that pulls in std, but we're unlikely to add new deps to this repo.
I wonder if something like https://github.com/cackle-rs/cackle could detect it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @leighmcculloch, I have integrated version 0.0.11 into Ledger, and it's working very well. Currently, I want to make rs-stellar-xdr support no std, so I will work on the tests here and consider adding no alloc support in my free time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's nothing riding on no alloc, so if it's not of value to you, I suggest holding off as there's nothing that I know of needing it right now.