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

@uviews macro hygiene issue #18

Merged
merged 2 commits into from
Aug 18, 2023
Merged

@uviews macro hygiene issue #18

merged 2 commits into from
Aug 18, 2023

Conversation

jondeuce
Copy link
Contributor

@jondeuce jondeuce commented Aug 15, 2023

The @uviews macro fails if the UnsafeArrays module symbol is not available in the current scope. MWE:

using UnsafeArrays: @uviews
A = rand(3, 3)
@uviews A view(A, 1, :) # UndefVarError: `UnsafeArrays` not defined

The issue is that when the macro builds the expression, it refers to UnsafeArrays.uview.

I fixed it by just interpolating $UnsafeArrays.uview instead, and modified the tests to run inside a dummy module where UnsafeArrays is not defined. I also fixed a couple silently broken tests that were not being run.

@oschulz
Copy link
Collaborator

oschulz commented Aug 17, 2023

Thanks @jondeuce !

@codecov
Copy link

codecov bot commented Aug 17, 2023

Codecov Report

Merging #18 (aaea586) into main (bfe7e49) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main      #18   +/-   ##
=======================================
  Coverage   99.22%   99.22%           
=======================================
  Files           4        4           
  Lines         129      129           
=======================================
  Hits          128      128           
  Misses          1        1           
Files Changed Coverage Δ
src/uview.jl 97.77% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@oschulz oschulz merged commit 58aff45 into JuliaArrays:main Aug 18, 2023
@oschulz
Copy link
Collaborator

oschulz commented Aug 18, 2023

JuliaRegistries/General#89903

Thanks again, @jondeuce !

@jondeuce
Copy link
Contributor Author

no problem! it's a great package 👍

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