hdf5: mark types with pointers #35
Conversation
| func (t *Datatype) Copy() (*Datatype, error) { | ||
| return copyDatatype(t.id) | ||
| c, err := copyDatatype(t.id) |
There was a problem hiding this comment.
perhaps modify the signature of copyDatatype to take a *Datatype instead of C.hid_t so nobody would unadvertantly drop the _hasGoPointer value on the floor?
There was a problem hiding this comment.
Sure, I'll do that when I make NewDatatype do the same thing with Identifier (and check where there are others that may also benefit.
h5t_types.go
Outdated
| @@ -462,7 +471,13 @@ func NewDataTypeFromType(t reflect.Type) (*Datatype, error) { | |||
| dt = &cdt.Datatype | |||
|
|
|||
| case reflect.Ptr: | |||
| return NewDataTypeFromType(t.Elem()) | |||
| dt, err = NewDataTypeFromType(t.Elem()) | |||
There was a problem hiding this comment.
I think that it is now time for us to start figuring out a way of handling H5T_STD_REF_OBJ...
There was a problem hiding this comment.
Do you want to file an issue for that?
|
Any comments about pointer path counting? |
|
I have added precise pointer chain accounting. PTAL |
h5t_types_test.go
Outdated
| } | ||
|
|
||
| for test := range tests { |
There was a problem hiding this comment.
This was testing int, test is the first var in the range statement.
| for test := range tests { | ||
| NewDatatypeFromValue(test) | ||
| // Test again for usage with ptrs | ||
| NewDatatypeFromValue(&test) |
There was a problem hiding this comment.
This would have been (but wasn't) testing *interface{} which we do not handle.
h5t_types.go
Outdated
| @@ -17,6 +17,8 @@ import ( | |||
|
|
|||
| type Datatype struct { | |||
| Identifier | |||
|
|
|||
| _goPtrPathLen int | |||
There was a problem hiding this comment.
Like @sbinet, I personally would name those fields w/o the underscore.
There was a problem hiding this comment.
Here it's OK to change to the w/o. Previously I preferred it the other way. I'll make the change.
| NewDatatypeFromValue(test) | ||
| // Test again for usage with ptrs | ||
| NewDatatypeFromValue(&test) | ||
| for _, test := range tests { |
There was a problem hiding this comment.
I was trying to figure out why my test was not working and shoved in a print %T and only got integers.
h5t_types_test.go
Outdated
| // Test again for usage with ptrs | ||
| NewDatatypeFromValue(&test) | ||
| for _, test := range tests { | ||
| dt, _ := NewDatatypeFromValue(test.v) |
There was a problem hiding this comment.
Any reason to not check for the error here?
There was a problem hiding this comment.
We'll panic later if we don't succeed. I'll change it.
|
PTAL |
And slice and array types.
| n := t.NumField() | ||
| for i := 0; i < n; i++ { | ||
| f := t.Field(i) | ||
| var field_dt *Datatype = nil | ||
| var field_dt *Datatype |
There was a problem hiding this comment.
I know you didn't wrote this in the first place, but maybe change to fieldDt? Maybe I should also open an issue for moving to Go-style Camel Case?
There was a problem hiding this comment.
There are a collection of things like this that I'd like to fix up. Please do file that issue.
This is part of the marking needed to know whether data copying will be needed. Note that it currently is overly conservative, and for all practical uses it will mark everything. There is a TODO that has an approach that I think will fix that.
This is more a PR for discussion of approach, but may end up being something that we can merge.
The half of the story needed to handle a
Datatypethat has been returned from opening a file is not dealt with here.Please take a look.
Updates #12.