hscan adds support for i386 platform

[monkey92t]

set: GOARCH=386

redis 127.0.0.1:6379>>set a 100
redis 127.0.0.1:6379>>set b 123456789123456789

type Demo struct {
    A int8 `redis:"a"`
    B int64 `redis:"b"`
}

client := redis.NewClient(&Options{
        Network:      "tcp",
        Addr:         "127.0.0.1:6379",
})  
ctx := context.Background()
d := &Demo{}
err := client.MGet(ctx, "a", "b").Scan(d)
t.Log(d, err)

it should run normally on the i386 platform, and there should not be such an error: strconv.ParseInt: parsing "123456789123456789": value out of range

[vmihailenco]

This looks good. I did not event know strconv.ParseInt(s, 10, 0) is used to parse int :)

Leaving open until tomorrow so @knadh have a chance to look at this.

[knadh]

Looks good @monkey92t.

Suggestion: Instead of separate decodeUint() and decodeUint64() (same for int) functions, there could just be one function that takes the bitSize arg. eg: decodeUint(f reflect.Value, s string, bitSize)

[monkey92t]

i found other problems:

for example:

type Temp struct {
	ID int8 `redis:"id"`
}
var t Temp

redis 127.0.0.1:6379>>set id 100
err := client.MGet(ctx, "id").Scan(&t)
//success, err = nil、t.ID = 100

//but:
redis 127.0.0.1:6379>>set id 999
err := client.MGet(ctx, "id").Scan(&t)
//err = nil 
//error result:  t.ID = -25 ???

the user may have unexpected results, err did not show an error,
if the number is out of range, should it be truncated or return an error, @knadh @vmihailenco what do you think.

[monkey92t]

看起来不错@ monkey92t。

建议：不要单独decodeUint()和decodeUint64()（同为INT）功能，有可能仅仅是一个函数，它的bitSizeARG。例如：decodeUint(f reflect.Value, s string, bitSize)

i plan to convert int8/16/32/64 to int64...
i think about this question again

[monkey92t]

we should consider using reflect.OverflowInt, reflect.OverflowUint, reflect. OverflowFloat ???

[knadh]

I don't think the errors should be silenced with bools indicating success or failure. You could still incorporate overflow checks and return a new error on that, right?

Example:

func decodeFloat(f reflect.Value, s string) error {
	v, err := strconv.ParseFloat(s, 64)
	if err != nil {
		return err
	}
	if f.OverflowFloat(v) {
		return errors.New("float overflows")
	}

	f.SetFloat(v)
	return true
}

[vmihailenco]

I agree that a302f11 does not look right and should be reverted.

I think all we need is a separate function for int8, int16 etc that passes the correct bigSize, e.g.

func decodeInt8(v reflect.Value, s string) error {
    return decodeInt(v, s, 8)
}

func decodeInt16(v reflect.Value, s string) error {
    return decodeInt(v, s, 16)
}

func decodeInt(v reflect.Value, s string, bitSize int) error {
	n, err := strconv.ParseInt(s, 10, bitSize)
        if err != nil {
            return err
        }
        v.SetInt(n)
        return nil
}

That should handle overflows.

[monkey92t]

i did not delete the error, is to concentrate the error on StructValue.Scan, if field.fn(s.value.Field(field.index), value) returns false, it is considered that it cannot be Scan

func (s StructValue) Scan(key string, value string) (err error) {
	field, ok := s.spec.m[key]
	if ok && !field.fn(s.value.Field(field.index), value) {
		t := s.value.Type()
		err = fmt.Errorf("cannot scan redis.result %s into struct field %s.%s of type %s",
			value, t.Name(), t.Field(field.index).Name, t.Field(field.index).Type)
	}
	return
}

[vmihailenco]

@monkey92t true, but it is also true that the original error is dropped (which is not nice).

[monkey92t]

@vmihailenco yes, i also hesitated, but i think the error message returned by decoderFunc func(reflect.Value, string) error is too vague,

Error Example:

redis 127.0.0.1:6379>> set id "abc"
type Temp struct {
	ID int `redis:"id"`
}
var t Temp

// before modification, err == `strconv.ParseInt: parsing "abc": invalid syntax`, is a vague reminder to the user
err := client.Mget(ctx, "id").Scan(&t)

// after modification, err == `cannot scan redis.result abc into struct field Temp.ID of type int`,  this is good tip
err := client.Mget(ctx, "id").Scan(&t)

decoderFunc func(reflect.Value, string) error does not contain Temp struct information field Temp.ID information, so it cannot handle very obvious error messages.
func(reflect.Value, string) error >> func(reflect.Value, string) bool is just a hidden method in the package and will not affect the outside.

[monkey92t]

If you do not change the return value type of decoderFunc func(reflect.Value, string) error, i think the Scan function only judges that err != nil, and does not use his return value information,

exp (field.fn(..) != nil):

func (s StructValue) Scan(key string, value string) (err error) {
	field, ok := s.spec.m[key]
	if ok && field.fn(s.value.Field(field.index), value) != nil {
		t := s.value.Type()
		err = fmt.Errorf("cannot scan redis.result %s into struct field %s.%s of type %s",
			value, t.Name(), t.Field(field.index).Name, t.Field(field.index).Type)
	}
	return
}

if you agree with this approach, i will send a PR to modify the return value type of decoderFunc

[monkey92t]

i restored the return value of decoderFunc, the error looks like this:
cannot scan redis.result 128 into struct field Temp.ID of type int8, error-int overflows

int overflows is the error message returned by decoderFunc

[monkey92t]

i adjusted the function according to your opinion, i am used to returning in the end in short functions

[monkey92t]

why do i encounter data race test errors many times? Go / Build (pull_request) Failing after 5m — Build, this should have nothing to do with hscan. should we investigate further?

[vmihailenco]

should we investigate further?

We should and I've tried, but without any luck. It looks like there is a race in the conn pool reaper, but I don't understand how that is possible...

[vmihailenco]

Thanks - looks good 👍

[knadh]

It looks like there is a race in the conn pool reaper

@vmihailenco Could you please point to where one could start investigating this?

[vmihailenco]

@knadh I've filed #1657 . It looks there are multiple data races which looks suspicious... perhaps our unsafe magic is not working any more...