Skip to content

Make TypeId as a key of TypeInfo#415

Merged
baszalmstra merged 17 commits intomun-lang:mainfrom
baszalmstra:feat/type_id_simplify
Jul 7, 2022
Merged

Make TypeId as a key of TypeInfo#415
baszalmstra merged 17 commits intomun-lang:mainfrom
baszalmstra:feat/type_id_simplify

Conversation

@baszalmstra
Copy link
Copy Markdown
Collaborator

This is an attempt to simplify the runtime and ABI by removing the use of TypeId in several places and only using Arc<TypeInfo>. This should enable adding pointer type TypeIds.

The idea is that some types are "concrete" which means they are concrete types like a struct or primitive. Pointers (and later arrays) are not concrete types but instead refer indirectly to concrete types.

The tests for the ABI and memory crate succeed, next up is modifying the compiler.

@Wodann Wodann self-requested a review June 29, 2022 01:19
Copy link
Copy Markdown
Collaborator

@Wodann Wodann left a comment

Choose a reason for hiding this comment

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

Partial review

@codecov
Copy link
Copy Markdown

codecov bot commented Jul 2, 2022

Codecov Report

Merging #415 (121e1c3) into main (7b54057) will decrease coverage by 0.13%.
The diff coverage is 83.49%.

@@            Coverage Diff             @@
##             main     #415      +/-   ##
==========================================
- Coverage   83.81%   83.68%   -0.14%     
==========================================
  Files         273      276       +3     
  Lines       16179    16405     +226     
==========================================
+ Hits        13560    13728     +168     
- Misses       2619     2677      +58     
Impacted Files Coverage Δ
crates/mun/tests/integration.rs 100.00% <ø> (ø)
crates/mun_abi/src/type_id.rs 0.00% <0.00%> (ø)
crates/mun_codegen/src/ir.rs 44.11% <ø> (+2.45%) ⬆️
crates/mun_codegen/src/ir/types.rs 33.33% <ø> (-23.81%) ⬇️
crates/mun_memory/src/gc/root_ptr.rs 66.66% <ø> (ø)
crates/mun_memory/src/mapping.rs 100.00% <ø> (ø)
crates/mun_runtime/src/reflection.rs 50.00% <33.33%> (-10.00%) ⬇️
crates/mun_codegen/src/value/int_value.rs 64.00% <40.00%> (-2.67%) ⬇️
crates/mun_abi/src/primitive.rs 46.66% <46.66%> (ø)
crates/mun_codegen/src/code_gen/symbols/mod.rs 74.48% <62.06%> (ø)
... and 55 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 191560f...121e1c3. Read the comment docs.

Copy link
Copy Markdown
Collaborator

@Wodann Wodann left a comment

Choose a reason for hiding this comment

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

Still WIP review

Copy link
Copy Markdown
Collaborator

@Wodann Wodann left a comment

Choose a reason for hiding this comment

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

First full pass on review is done

@baszalmstra
Copy link
Copy Markdown
Collaborator Author

baszalmstra commented Jul 5, 2022

I updated the c++ code as well! This all seems to pass CI now too!

Can you check the comments I responded to @Wodann ?

Im still planning on adding a few tests here and there.

@Wodann Wodann marked this pull request as ready for review July 7, 2022 03:57
@Wodann Wodann self-requested a review July 7, 2022 03:57
Copy link
Copy Markdown
Collaborator

@Wodann Wodann left a comment

Choose a reason for hiding this comment

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

Some minor nitpicks. Once you've readded some of the tests too, it looks good to me! Awesome work 😃

@baszalmstra
Copy link
Copy Markdown
Collaborator Author

I fixed some of your final comments, it looks good to me too! @Wodann can you please review my last comments and code? If you're in agreement, feel free to merge! :)

@baszalmstra baszalmstra merged commit 1fff041 into mun-lang:main Jul 7, 2022
@Wodann Wodann added this to the Mun v0.4.0 milestone Jul 8, 2022
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