Skip to content

Store effects in OptimizationSource#281

Merged
aviatesk merged 1 commit intoJuliaDebug:masterfrom
ianatol:ia/optsrceffects
Apr 7, 2022
Merged

Store effects in OptimizationSource#281
aviatesk merged 1 commit intoJuliaDebug:masterfrom
ianatol:ia/optsrceffects

Conversation

@ianatol
Copy link
Member

@ianatol ianatol commented Mar 22, 2022

@Keno pointed out to me that https://github.com/JuliaDebug/Cthulhu.jl/blob/master/src/Cthulhu.jl#L343 was broken. To fix, we want to store effects in OptimizationSource and access them from there instead of the current, faulty way.

@codecov-commenter
Copy link

codecov-commenter commented Mar 22, 2022

Codecov Report

Merging #281 (019f6d5) into master (00415a4) will increase coverage by 0.42%.
The diff coverage is 76.92%.

❗ Current head 019f6d5 differs from pull request most recent head 9d86941. Consider uploading reports for the commit 9d86941 to get more accurate results

@@            Coverage Diff             @@
##           master     #281      +/-   ##
==========================================
+ Coverage   56.23%   56.65%   +0.42%     
==========================================
  Files           7        7              
  Lines        1115     1112       -3     
==========================================
+ Hits          627      630       +3     
+ Misses        488      482       -6     
Impacted Files Coverage Δ
src/Cthulhu.jl 36.66% <0.00%> (ø)
src/interpreter.jl 92.15% <83.33%> (-4.14%) ⬇️
src/codeview.jl 62.50% <0.00%> (+6.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 00415a4...9d86941. Read the comment docs.

@ianatol
Copy link
Member Author

ianatol commented Mar 28, 2022

Should be good to go now that JuliaLang/julia#44777 is merged

Edit: More importantly, Cthulhu is broken on master now that JuliaLang/julia#44777 is merged

@simeonschaub
Copy link
Collaborator

Does that PR need to be backported? Otherwise, Cthulhu would be broken on 1.8, right?

@ianatol
Copy link
Member Author

ianatol commented Mar 28, 2022

Hmm yeah, we would want to backport it for that effects printing to properly work on 1.8. It would also be a bit cleaner to just check EFFECTS_ENABLED rather than the version number.

@aviatesk aviatesk self-requested a review March 29, 2022 01:42
@ianatol
Copy link
Member Author

ianatol commented Mar 30, 2022

Rebased with #285

Co-authored-by: Shuhei Kadowaki <40514306+aviatesk@users.noreply.github.com>
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.

5 participants