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

refactor: use Idx<T> instead of macro in arena #266

Merged
merged 1 commit into from
Sep 11, 2020

Conversation

baszalmstra
Copy link
Collaborator

This PR refactors the Arena Id's a little bit so that we need fewer macros and get a clearer coupling between Arenas, their data and their ids. Similar to how rust analyzer refactored this.

I also renamed a few Ids to indicate that they are "Local" to their container (TypeRefId is local to a specific TypeRefMap and is now called LocalTypRefId). I found that to be clearer.

@baszalmstra baszalmstra added the type: refactor Refactor existing code label Sep 11, 2020
@baszalmstra baszalmstra requested a review from Wodann September 11, 2020 12:47
@baszalmstra baszalmstra self-assigned this Sep 11, 2020
@codecov
Copy link

codecov bot commented Sep 11, 2020

Codecov Report

Merging #266 into master will increase coverage by 0.33%.
The diff coverage is 76.38%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #266      +/-   ##
==========================================
+ Coverage   78.75%   79.08%   +0.33%     
==========================================
  Files         215      214       -1     
  Lines       12857    12803      -54     
==========================================
  Hits        10125    10125              
+ Misses       2732     2678      -54     
Impacted Files Coverage Δ
crates/mun_codegen/src/ir/body.rs 84.23% <ø> (-0.14%) ⬇️
crates/mun_hir/src/adt.rs 90.00% <ø> (ø)
crates/mun_hir/src/resolve.rs 86.11% <ø> (ø)
crates/mun_hir/src/source_id.rs 80.00% <ø> (ø)
crates/mun_hir/src/arena/map.rs 64.28% <61.90%> (-9.05%) ⬇️
crates/mun_hir/src/arena.rs 59.57% <72.72%> (-4.32%) ⬇️
crates/mun_hir/src/expr.rs 87.65% <75.00%> (ø)
crates/mun_hir/src/expr/scope.rs 93.75% <88.88%> (ø)
crates/mun_hir/src/type_ref.rs 61.36% <88.88%> (ø)
crates/mun_hir/src/code_model.rs 79.73% <100.00%> (ø)
... and 13 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 d9ebc18...9c93702. Read the comment docs.

@Wodann
Copy link
Collaborator

Wodann commented Sep 11, 2020

My only concern is the reduced codecov, otherwise it looks good.

It seems that codecov already has some good recommendations for improving the coverage.

@baszalmstra baszalmstra merged commit e330f15 into mun-lang:master Sep 11, 2020
@baszalmstra
Copy link
Collaborator Author

My only concern is the reduced codecov, otherwise it looks good.

It seems that codecov already has some good recommendations for improving the coverage.

Oh I only just noticed you also commented. The code coverage of the overall project actually increases because the MR removes a lot of code. Other than that (besides the Arena code, which is tested) I only really renamed stuff so I didn't feel like adding more tests would be necessary.

I do see that there are a few functions in there that dont seem to be used at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: refactor Refactor existing code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants