hdf: Add support for pointers in NewDataTypeFromType() #15
Conversation
| @@ -466,6 +466,9 @@ func NewDataTypeFromType(t reflect.Type) (*Datatype, error) { | |||
| } | |||
| dt = &cdt.Datatype | |||
|
|
|||
| case reflect.Ptr: | |||
| return NewDataTypeFromType(t.Elem()) | |||
There was a problem hiding this comment.
if t is *int, this will return the HDF5 DataType for int.
shouldn't we instead return the HDF5 DataType that describes a pointer to int ?
There was a problem hiding this comment.
This type specification is in line with the current implementation of Append:
https://github.com/gonum/hdf5/blob/master/h5pt.go#L193-L195
case reflect.Ptr:
ptrVal := rp.Elem()
cData = unsafe.Pointer(&ptrVal)
If you propose to use H5T_STD_REF_OBJ as a type of *int and then add the actual int to a separate file location the Append function (possibly among many others) would need to be refactored as well.
The change as is now, prevents a panic in default and works as intended.
There was a problem hiding this comment.
ah. right you are :)
could we add a test exercizing this new code path?
thanks.
There was a problem hiding this comment.
I fixed the test cases. I think they are testing now the stuff that should be tested.
|
Have you had time to check it out @sbinet? 👻 I think there will be another patch coming in shortly, as well... Support for arrays/slices and nested struct appends. But I want to have that one closed first 😄 |
h5t_test.go
Outdated
| @@ -65,48 +68,62 @@ func TestStructDatatype(t *testing.T) { | |||
|
|
|||
| // Test that the type can be constructed and that the number of | |||
| // members is as expected. | |||
| dtypes := make([]*Datatype, 0) | |||
There was a problem hiding this comment.
we usually spell this out as:
var dtypes []*DatatypeThere was a problem hiding this comment.
If it is really pressing, I can obviously fix that one.
|
Alrighty. Amended the commit. Is now aligned with your usual spelling. |
thanks :)
If I am not mistaken, @gustafj did commit something in the first commit, so, yeah, both of you will need to send a PR against gonum/gonum |
|
As I am already in there, it will just be him. Will fix shortly :) Thx! |
This small patch introduces support for pointers to datatypes in the NewDataTypeFromType function. This enables support for different use cases, e.g. usage with google.com/gopacket Co-authored-by: Gustaf Johansson <gustaf@pinon.se>
implementation. As the current implementation does not allow the usage of H5T_STD_REF_OBJ, these tests only test for the case of pointers to values in computer memory (opposed to pointers that point to a location inside an HDF5 file). Co-authored-by: Gustaf Johansson <gustaf@pinon.se>
|
PR added. + I fixed the commits. :) |
|
Just a quick reminder @sbinet :)) |
|
Actually, we are now waiting on @kortschak :) If he doesn't but by Monday (when I come back from holidays) I'll merge this in. |
|
No worries. |
This small patch introduces support for pointers to datatypes in the
NewDataTypeFromType function. This enables support for different use
cases, e.g. usage with google.com/gopacket
Co-authored-by: Gustaf Johansson gustaf.j@gmail.com