lunixbochs / struc

Better binary packing for Go
MIT License
564 stars 42 forks source link

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

Closed 13rac1 closed 5 years ago

13rac1 commented 5 years ago

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

lunixbochs commented 5 years ago

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

13rac1 commented 5 years ago

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

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

13rac1 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

13rac1 commented 5 years ago

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. https://github.com/lunixbochs/struc/blob/02e4c2afbb2ac4bae6876f52c8273fc4cf5a4b0a/custom_test.go#L74-L78

13rac1 commented 5 years ago

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

13rac1 commented 5 years ago

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 commented 5 years ago

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.