unsafe_wrap for symbols

[vchuravy]

Spiritual follow-up on #2624

Noticed while working on trixi-framework/Trixi.jl#2212

[github-actions]

Your PR requires formatting changes to meet the project's style guidelines.
Please consider running Runic (git runic master) to apply these changes.

Click here to view the suggested changes.

diff --git a/src/array.jl b/src/array.jl
index 2414768ae..412bd8978 100644
--- a/src/array.jl
+++ b/src/array.jl
@@ -50,21 +50,21 @@ end
 # As well as "mutable singleton" types like `Symbol` that use pointer-identity
 
 function valid_leaftype(@nospecialize(T))
-  Base.allocatedinline(T) || (Base.ismutabletype(T) && Base.datatype_fieldcount(T) == 0)
+    return Base.allocatedinline(T) || (Base.ismutabletype(T) && Base.datatype_fieldcount(T) == 0)
 end
 
 function valid_type(@nospecialize(T))
-  if valid_leaftype(T)
-    if hasfieldcount(T)
-      return all(valid_type, fieldtypes(T))
+    if valid_leaftype(T)
+        if hasfieldcount(T)
+            return all(valid_type, fieldtypes(T))
+        end
+        return true
     end
-    return true
-  end
-  return false
+    return false
 end
 
-@inline function check_eltype(name, T)                      
-  if !valid_type(T) 
+@inline function check_eltype(name, T)
+    return if !valid_type(T)
     explanation = explain_eltype(T)
     error("""
       $name only supports element types that are allocated inline.
@@ -249,7 +249,7 @@ end
 function Base.unsafe_wrap(::Type{CuArray{T,N,M}},
                           ptr::CuPtr{T}, dims::NTuple{N,Int};
                           own::Bool=false, ctx::CuContext=context()) where {T,N,M}
-  check_eltype("unsafe_wrap(CuArray, ...)", T)
+    check_eltype("unsafe_wrap(CuArray, ...)", T)
   sz = prod(dims) * aligned_sizeof(T)
 
   # create a memory object
diff --git a/test/base/array.jl b/test/base/array.jl
index 42f4668af..83335046b 100644
--- a/test/base/array.jl
+++ b/test/base/array.jl
@@ -176,14 +176,14 @@ end
 
     # symbols and tuples thereof
     let a = CuArray([:a])
-      b = unsafe_wrap(CuArray, pointer(a), 1)
-      @test typeof(b) <: CuArray{Symbol,1}
-      @test size(b) == (1,)
+        b = unsafe_wrap(CuArray, pointer(a), 1)
+        @test typeof(b) <: CuArray{Symbol, 1}
+        @test size(b) == (1,)
     end
-    let a = CuArray([(:a,:b)])
-      b = unsafe_wrap(CuArray, pointer(a), 1)
-      @test typeof(b) <: CuArray{Tuple{Symbol,Symbol},1}
-      @test size(b) == (1,)
+    let a = CuArray([(:a, :b)])
+        b = unsafe_wrap(CuArray, pointer(a), 1)
+        @test typeof(b) <: CuArray{Tuple{Symbol, Symbol}, 1}
+        @test size(b) == (1,)
     end
 end
 

[maleadt]

CI failures related.

[vchuravy]

Fun!

This fails:

CuArray([:a])

but this doesn't

CuArray([(:a,)])

The latter being the one I tested locally

[maleadt]

Having s/sizeof/argsize all over the wrappers feels very wrong. That was supposed to be an isolated thing for use during argument conversion.

[vchuravy]

Yeah, that's why I reverted the commit that added it to memory.jl

The crux is that we need a version of sizeof that returns sizeof(Ptr{Cvoid}) instead of sizeof(Symbol) == nothing.

I am doubly amused that :

julia> sizeof(Tuple{Symbol})
8

Of course works...

I am half tempted to shadow sizeof inside CUDA.jl with an arguably correct implementation.
Or we add something like morally_correct_sizeof to GPUToolkit.jl.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.55%. Comparing base (5645ddc) to head (55fde61).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2753   +/-   ##
=======================================
  Coverage   89.54%   89.55%           
=======================================
  Files         153      153           
  Lines       13182    13189    +7     
=======================================
+ Hits        11804    11811    +7     
  Misses       1378     1378           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[maleadt]

Why also check for Base.ismutabletype(T) here? Any reason this shouldn't cover immutable fieldless types; those should also be valid, no?

I'm also slightly confused by this being called leaftype while it's the very first check done on the outer type too, but maybe that's just a naming issue.

[vchuravy]

ismutabletype is excluding the Union & co types.

I'm also slightly confused by this being called leaftype while it's the very first check done on the outer type too, but maybe that's just a naming issue.

Yeah, I could probably inline this back into valid_type. I was trying to restructure as one having the logic and the other being the recursion. Maybe it ought to be something like valid_concrete_type.