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

Allow option to opt-out of provided memory allocator #1661

Merged
merged 8 commits into from
Feb 21, 2023
Merged

Conversation

HCastano
Copy link
Contributor

@HCastano HCastano commented Feb 19, 2023

This takes #1415 one step further and provides the
no-allocator feature to the top level ink crate, allowing contract authors to opt-out
of the provided memory allocator.

It also adds an example showing how a contract author could provide their own allocator.

Closes #1414.

cc @kvinwang

@codecov-commenter
Copy link

codecov-commenter commented Feb 19, 2023

Codecov Report

Merging #1661 (ebbac25) into master (b8862a1) will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1661      +/-   ##
==========================================
+ Coverage   70.72%   70.74%   +0.01%     
==========================================
  Files         206      206              
  Lines        6416     6416              
==========================================
+ Hits         4538     4539       +1     
+ Misses       1878     1877       -1     
Impacted Files Coverage Δ
crates/allocator/src/bump.rs 87.60% <0.00%> (ø)
crates/ink/ir/src/ir/attrs.rs 82.02% <0.00%> (+0.28%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@@ -0,0 +1,180 @@
//! # Custom Allocator
//!
//! This example demonstrates how to opt-out of the ink! provided global memory allocator.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Lemme be the guy commenting about something on the order of 80 CPL 😬 .

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we configure rustfmt to catch this? I think it enforces code CPL to 100, not sure about the comments.

Copy link
Contributor Author

@HCastano HCastano Feb 21, 2023

Choose a reason for hiding this comment

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

We already enforce this to 90 characters, which is what it's at 😉

https://github.com/paritytech/ink/blob/b8862a1147a8fb577c82304ed1dec9a8c45c386a/.rustfmt.toml#L1

Edit:

Actually, we should be enforcing comments as 80

https://github.com/paritytech/ink/blob/b8862a1147a8fb577c82304ed1dec9a8c45c386a/.rustfmt.toml#L9

So maybe there is something to look into here 🤔

But everything else is under 100 which is reasonable imo.

Copy link
Collaborator

@cmichi cmichi left a comment

Choose a reason for hiding this comment

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

LGTM. Just the nitpicks about having less CPL throughout.

@cmichi
Copy link
Collaborator

cmichi commented Feb 20, 2023

Add the example to https://github.com/paritytech/ink-examples as well, please.

@HCastano HCastano merged commit 90aad92 into master Feb 21, 2023
@HCastano HCastano deleted the hc-no-allocator branch February 21, 2023 02:50
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.

Consider add dlmalloc-rs as allocator or opt-out the BumpAllocator
4 participants