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

Support packing/unpacking of custom slices, adds tests #63

Closed
wants to merge 6 commits into from
Closed

Support packing/unpacking of custom slices, adds tests #63

wants to merge 6 commits into from

Conversation

13rac1
Copy link
Contributor

@13rac1 13rac1 commented Mar 17, 2019

Allows custom types containing slices to handle their own elem
processing. Does not affect slices of a custom type.

Useful for implementing e.g. NULL-terminated slices whose length
is unknown and cannot be determined by a field in the struct.

type IntSlice []int
type MyInt int
type Fields struct {
    intsA IntSlice // Handles own, fd.Slice = false
    intsB []MyInt  // Doesn't handle own, fd.Slice = true
}

This is one method to inform the parsing process of the different
requirements. I'm not sure it is ideal. Another option is new
method, such as HandlesElements() on the Custom interface.

Closes #60

Allows custom types containing slices to handle their own elem
processing. Does not affect slices of a custom type.

Useful for implementing e.g. NULL-terminated slices whose length
is unknown and cannot be determined by a field in the struct.

    type IntSlice []int
    type MyInt int
    type Fields struct {
        intsA IntSlice // Handles own, fd.Slice = false
        intsB []MyInt  // Doesn't handle own, fd.Slice = true
    }

This is one method to inform the parsing process of the different
requirements. I'm not sure it is ideal. Another option is new
method, such as `HandlesElements()` on the Custom interface.

Closes #60

Co-authored-by: Robin Eklind <mewmew@users.noreply.github.com>
@lunixbochs
Copy link
Owner

Cool! I'd like to figure out how to not use String() though

@13rac1
Copy link
Contributor Author

13rac1 commented Mar 17, 2019

Same! I'm open to any suggestions to avoid that.

🤔 I should implement a test taking the other code path, perhaps that exercise will make it more clear.

parse.go Outdated Show resolved Hide resolved
@13rac1
Copy link
Contributor Author

13rac1 commented Mar 17, 2019

Option 3: If we make better error messaging, it could be an additional struct tag. That's a non-API breaking option. Edit: But that is annoyingly redundant since the custom type already "knows."

@lunixbochs
Copy link
Owner

lunixbochs commented Mar 17, 2019

I feel like the answer is in here:

package main

import (
    "fmt"
    "reflect"
)

type Interface interface {
    Blah() int
}

type BlahImpl struct{}

func (b *BlahImpl) Blah() int { return 1 }

type Test []int

func (t *Test) Blah() int { return 1 }

type Struct struct {
    CustomSlice   Test
    SliceOfCustom []BlahImpl
}

func main() {
    test := Struct{}

    abstract := reflect.TypeOf((*Interface)(nil)).Elem()
    fmt.Println(abstract)

    val := reflect.ValueOf(test)
    for i := 0; i < val.NumField(); i++ {
        f := val.Field(i)
        fmt.Println(reflect.PtrTo(f.Type()).Implements(abstract), reflect.PtrTo(f.Type().Elem()).Implements(abstract))
    }
}

@lunixbochs
Copy link
Owner

lunixbochs commented Mar 17, 2019

tests pass on the test-63 branch but I feel uneasy, can you do more testing with both "custom slice types" and "slices of custom types" and look for holes/regressions?

@13rac1
Copy link
Contributor Author

13rac1 commented Mar 17, 2019

This looks like a good option!

I'll add a test for the other code path, AKA []BlahImpl. Can convert it to table-drive tests also.

@lunixbochs
Copy link
Owner

lunixbochs commented Mar 17, 2019

Should also test type CustomType []CustomType2 where both implement Pack (only the outer type should be used by struc in this case)

@13rac1
Copy link
Contributor Author

13rac1 commented Mar 17, 2019

Just realized. There is already a commented out test for slices of custom types. They don't work. I'm going to add a panic to save someone from trying.

struc/custom_test.go

Lines 74 to 78 in 02e4c2a

// TODO: slices of custom types don't work yet
/*
type Int3SliceStruct struct {
I [2]Int3
}

Uses reflect to find arrays/slices containing custom types
Adds many permutation of using custom types, including testing for
the panic when using an array of custom types in a struct.

Note: The test ArrayOfCustomType should panic, but does not yet.

--- PASS: TestCustomTypes (0.00s)
    --- PASS: TestCustomTypes/CustomType (0.00s)
    --- PASS: TestCustomTypes/CustomType-Big (0.00s)
    --- PASS: TestCustomTypes/CustomTypeStruct (0.00s)
    --- PASS: TestCustomTypes/ArrayOfCustomType (0.00s)
    --- PASS: TestCustomTypes/ArrayOfCustomTypeStruct (0.00s)
    --- PASS: TestCustomTypes/CustomTypeOfArrayOfUInt8 (0.00s)
    --- PASS: TestCustomTypes/CustomTypeOfArrayOfUInt8Struct (0.00s)
    --- PASS: TestCustomTypes/CustomTypeOfSliceOfUInt8 (0.00s)
    --- PASS: TestCustomTypes/CustomTypeOfSliceOfUInt8-Empty (0.00s)
    --- PASS: TestCustomTypes/CustomTypeOfSliceOfUInt8Struct (0.00s)
@13rac1
Copy link
Contributor Author

13rac1 commented Mar 17, 2019

Argh. Build failed because of ancient Go.
old go fails

t.Run undefined (type *testing.T has no field or method Run)

t.Run() was made public in 1.7

custom_test.go Outdated Show resolved Hide resolved
One pass, one panic. The tests are correct. The panic() only occurs
because the detection of unimplemented "ArrayOfCustomType" is incorrect.
@13rac1
Copy link
Contributor Author

13rac1 commented Mar 19, 2019

No more purposefully broken tests. There are now 16 test cases to show what is supported and what is not. Unimplemented features remaining:

  1. Array of normal or custom types
  2. Struct of Array of custom types

I'd like to rebase/squash the commits, probably reducing it to 3-4 commits and call this good. What do you think?

FWIW I've been using this code since the first commit in my own project for null terminated byte/string fields, which I want to PR after this is merged.

Edit: I've just realized that Strings are handled uniquely, so will need their own set of tests

@13rac1
Copy link
Contributor Author

13rac1 commented Mar 26, 2019

Haven't heard from you in 10 days. I'm closing for the reduced scope of just adding table driven tests in #64 to avoid wasting the current mental context.

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