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 docstring <-> code mismatch of CompilerConfig #422

Merged
merged 1 commit into from
Apr 4, 2023

Conversation

Seelengrab
Copy link
Contributor

The docstring said entry_name, but the code used name as the field - this PR makes them consistently use entry_name.

@Seelengrab
Copy link
Contributor Author

Ah, forgot to change the uses in the backends 🤦

@maleadt
Copy link
Member

maleadt commented Apr 4, 2023

Maybe it's better to sync the other way? I decided against renaming the kwarg to entry_name because it seemed like a needlessly breaking change (but as you noticed, forgot to update the docstring).

@Seelengrab
Copy link
Contributor Author

Either way is fine by me - I couldn't figure out which was intended, since both argument names stem from the same commit :') I'll change it to name then, that should fix the backend tests as well

@Seelengrab
Copy link
Contributor Author

Error: The process '/usr/bin/git' failed with exit code 1

Yikes. 👀

@codecov
Copy link

codecov bot commented Apr 4, 2023

Codecov Report

Patch coverage has no change and project coverage change: -7.78 ⚠️

Comparison is base (f793923) 86.92% compared to head (f6b8cb2) 79.14%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #422      +/-   ##
==========================================
- Coverage   86.92%   79.14%   -7.78%     
==========================================
  Files          24       24              
  Lines        2922     2892      -30     
==========================================
- Hits         2540     2289     -251     
- Misses        382      603     +221     
Impacted Files Coverage Δ
src/interface.jl 80.95% <ø> (-5.96%) ⬇️

... and 12 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@maleadt maleadt merged commit d6384c7 into JuliaGPU:master Apr 4, 2023
@@ -114,12 +114,12 @@ module LazyCodegen
@assert !cc.compiled
job = cc.job

entry_name, jitted_mod = JuliaContext() do ctx
name, jitted_mod = JuliaContext() do ctx
Copy link
Member

@maleadt maleadt Apr 12, 2023

Choose a reason for hiding this comment

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

FWIW, this introduced a bug. By assigning to name, the call to LLVM.name in the block is broken.
Fixed in #427

Copy link
Contributor Author

@Seelengrab Seelengrab Apr 12, 2023

Choose a reason for hiding this comment

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

Odd that this wasn't caught in CI 🤔 But yeah, shows that a quick sed -i is often more dangerous than one might think 😬

Copy link
Member

Choose a reason for hiding this comment

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

It was caught, but resulted in a timeout which we ignored...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah :| Mea culpa then! Still surprising that the others ran through, but it is what it is. Good thing you caught & fixed it 👍

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