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

feat: add casting of fundamental types to memory mapping #140

Merged
merged 5 commits into from
Apr 29, 2020

Conversation

Wodann
Copy link
Collaborator

@Wodann Wodann commented Apr 23, 2020

Adds support for fundamental type conversions, when memory mapping. E.g. going from this struct:

struct Foo(
    u8,
    i16,
    u32,
    i64,
    f32,
)

to the following struct, is legal:

struct Foo(
    u16,
    i32,
    u64,
    i128,
    f64,
)

[Discussion] Non-guaranteed type conversions will result in a panic zero initialization. Is this desirable? Potentially we could at least attempt the conversion and only panic! zero initialize if the try_into call fails.

[Discussion] Non-legal or failed type conversions will result in a panic!. Is this desirable? Alternatives are:

  1. Logging an error and initialising to zero.
  2. Graciously fail the linking of the Assembly. Changes will have to be made to the source code before we re-evaluate the Assembly. This means that you can never "reuse" a field by doing an illegal conversion. This might force people to take a two-step hot reloading approach: i.e. remove first and then add the new field.

Edit: after discussion, we decided to never panic. If we cannot resolve the type conversion, we continue with the mapping - assuming zero initialization.

@Wodann Wodann requested a review from baszalmstra April 23, 2020 13:59
@Wodann Wodann self-assigned this Apr 23, 2020
@codecov
Copy link

codecov bot commented Apr 24, 2020

Codecov Report

Merging #140 into master will increase coverage by 0.26%.
The diff coverage is 96.15%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #140      +/-   ##
==========================================
+ Coverage   81.80%   82.06%   +0.26%     
==========================================
  Files         159      160       +1     
  Lines       10672    10806     +134     
==========================================
+ Hits         8730     8868     +138     
+ Misses       1942     1938       -4     
Impacted Files Coverage Δ
crates/mun/src/main.rs 0.00% <0.00%> (ø)
crates/mun_abi/src/lib.rs 92.85% <ø> (ø)
crates/mun_abi/src/static_type_map.rs 90.90% <ø> (ø)
crates/mun_runtime/src/reflection.rs 54.34% <ø> (ø)
crates/mun_runtime/src/lib.rs 71.73% <50.00%> (+0.77%) ⬆️
crates/mun_abi/src/type_info.rs 74.35% <77.77%> (ø)
crates/mun_abi/src/function_info.rs 100.00% <100.00%> (ø)
crates/mun_memory/src/cast.rs 100.00% <100.00%> (ø)
crates/mun_memory/src/gc/mark_sweep.rs 86.71% <100.00%> (+1.20%) ⬆️
crates/mun_memory/tests/diff/mod.rs 100.00% <100.00%> (ø)
... and 9 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 54580ca...6b4c47d. Read the comment docs.

@Wodann
Copy link
Collaborator Author

Wodann commented Apr 25, 2020

@baszalmstra any comments on the discussion topics in this description?

Wodann added 4 commits April 25, 2020 12:37
The HasStaticTypeInfo trait and FunctionInfoStorage struct are closely
related to the ABI, and multiple crates depend on them. Hence, it was
decided to move them up the dependency chain to mun_abi.
@baszalmstra
Copy link
Collaborator

baszalmstra commented Apr 26, 2020

[Discussion] Non-guaranteed type conversions will result in a panic. Is this desirable? Potentially we could at least attempt the conversion and only panic! if the try_into call fails.

I'd say that it should never panic! because I user can never catch that.

[Discussion] Non-legal or failed type conversions will result in a panic!. Is this desirable?

No, I don't think we should ever panic!. I think this is something that we have to play with before we can really make a decision. So far I think option number 1 is the most user friendly.

@Wodann
Copy link
Collaborator Author

Wodann commented Apr 26, 2020

I'll add a commit to init to zero, in that case 👌🏻

@Wodann
Copy link
Collaborator Author

Wodann commented Apr 28, 2020

I've updated the PR description to reflect that we decided to never panic! and added a separate commit to implement and test this behavior.

Could you review that commit please @baszalmstra ?

@Wodann Wodann requested a review from baszalmstra April 28, 2020 16:31
@Wodann Wodann merged commit ecca852 into mun-lang:master Apr 29, 2020
@Wodann Wodann mentioned this pull request May 4, 2020
@Wodann Wodann added this to the Mun v0.2 milestone May 14, 2020
@Wodann Wodann deleted the feature/cast branch May 14, 2020 15:12
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.

2 participants