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

Fix restack for struct with undef fields #498

Merged
merged 4 commits into from
Oct 20, 2021
Merged

Conversation

maartenvd
Copy link
Contributor

@maartenvd maartenvd commented Oct 15, 2021

this is the kind of 'fix' I had in mind, but it's not very pretty

close #497

this is the kind of 'fix' I had in mind, but it's not very pretty
@tkf
Copy link
Member

tkf commented Oct 15, 2021

Thanks for giving a shot at it! Yeah, we may need something like this. I'm a bit worried that it stresses the compiler too much (esp. for a large struct), though.

We can also check if all fields are isdefined and just return the input as-is if not. It may miss some optimization opportunities but it could be a better solution overall.

@codecov
Copy link

codecov bot commented Oct 15, 2021

Codecov Report

Merging #498 (8014fcd) into master (1e5e530) will decrease coverage by 0.89%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #498      +/-   ##
==========================================
- Coverage   93.40%   92.50%   -0.90%     
==========================================
  Files          32       32              
  Lines        2214     2215       +1     
==========================================
- Hits         2068     2049      -19     
- Misses        146      166      +20     
Flag Coverage Δ
Pkg.test 92.66% <100.00%> (+0.01%) ⬆️
Run.test 92.37% <100.00%> (-0.90%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/AutoObjectsReStacker.jl 95.65% <100.00%> (+0.65%) ⬆️
src/reduce.jl 72.56% <0.00%> (-9.89%) ⬇️
src/basics.jl 88.88% <0.00%> (-5.56%) ⬇️
src/partitionby.jl 90.90% <0.00%> (-3.90%) ⬇️
src/core.jl 89.62% <0.00%> (-0.95%) ⬇️
src/progress.jl 91.58% <0.00%> (-0.94%) ⬇️
src/library.jl 95.81% <0.00%> (+0.25%) ⬆️

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 1e5e530...8014fcd. Read the comment docs.

@maartenvd
Copy link
Contributor Author

I think I only have to check the last field, which is why I made that the outer loop in the original pull request. This version is now just the outer if, where I return x if it contains uninitialized fields

@maartenvd
Copy link
Contributor Author

@tkf - should I change it to check every field, or are you fine with just checking the last field - which should also tell us if it's a fully or partially initialized struct?

src/AutoObjectsReStacker.jl Outdated Show resolved Hide resolved
src/AutoObjectsReStacker.jl Outdated Show resolved Hide resolved
@tkf tkf changed the title attempt at fixing #497 Fix restack for struct with undef fields Oct 20, 2021
@tkf
Copy link
Member

tkf commented Oct 20, 2021

just checking the last field

Ah, good point! I think this works.

Can you add a test in https://github.com/JuliaFolds/Transducers.jl/blob/master/test/test_restack.jl ? I think it's good to go then.

@maartenvd
Copy link
Contributor Author

something like this? I want to use transducers in my own package, I'm getting promising results so far :)

@tkf tkf enabled auto-merge (squash) October 20, 2021 08:46
@tkf
Copy link
Member

tkf commented Oct 20, 2021

Thanks! LGTM!

Good to know that it works in your package :)

@tkf tkf merged commit 1f2ac2c into JuliaFolds:master Oct 20, 2021
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.

restack errors on partially initialized structs
2 participants