[#xx] Make all error status receivers pointers #135

ale64bit · 2023-08-02T07:02:12Z

ale64bit commented

2023-08-02 07:02:12 +00:00

See TrueCloudLab/frostfs-node#481

Signed-off-by: Alejandro Lopez a.lopez@yadro.com

See https://git.frostfs.info/TrueCloudLab/frostfs-node/issues/481 Signed-off-by: Alejandro Lopez <a.lopez@yadro.com>

ale64bit force-pushed fix/status-error-pointer from 9073aaa146 to fe35170789

2023-08-02 07:07:35 +00:00

Compare

ale64bit changed title from ~~[#xx] Make all error status receivers pointers~~ to WIP: [#xx] Make all error status receivers pointers

2023-08-02 07:08:09 +00:00

ale64bit changed title from ~~WIP: [#xx] Make all error status receivers pointers~~ to [#xx] Make all error status receivers pointers

2023-08-02 07:08:16 +00:00

ale64bit requested review from storage-core-committers 2023-08-02 07:08:31 +00:00

ale64bit requested review from storage-core-developers 2023-08-02 07:08:32 +00:00

aarifullin approved these changes 2023-08-02 07:30:53 +00:00

aarifullin left a comment

Nice👍 We really need these changes

acid-ant approved these changes 2023-08-02 07:54:38 +00:00

fyrchik requested review from storage-services-developers 2023-08-02 14:27:52 +00:00

fyrchik requested review from storage-services-committers 2023-08-02 14:27:53 +00:00

fyrchik reviewed 2023-08-02 14:31:19 +00:00

client/errors.go Outdated

					
				@ -9,4 +9,1 @@

				// unwraps err using errors.Unwrap and returns the result.

				func unwrapErr(err error) error {

					for e := errors.Unwrap(err); e != nil; e = errors.Unwrap(err) {

fyrchik commented

2023-08-02 14:28:44 +00:00

Why did you remove this logic?

ale64bit commented

2023-08-02 14:51:43 +00:00

because it's not used and it's not exported

fyrchik commented

2023-08-03 06:59:01 +00:00

Ok, why did we not use unwrapErr in IsErr...() anymore?

It was a deliberate decision and I don't see how this change is related to "make all status receivers pointers".
See for the discussion when this was introduced https://github.com/nspcc-dev/neofs-sdk-go/pull/279#discussion_r907279144

Ok, why did we not use `unwrapErr` in `IsErr...()` anymore? It was a deliberate decision and I don't see how this change is related to "make all status receivers pointers". See for the discussion when this was introduced https://github.com/nspcc-dev/neofs-sdk-go/pull/279#discussion_r907279144

ale64bit commented

2023-08-04 08:24:47 +00:00

discussed offline

fyrchik marked this conversation as resolved

client/errors.go Outdated

					
				@ -28,2 +13,2 @@

						return true

					}

					target := new(apistatus.ContainerNotFound)

					return errors.As(err, &target)

fyrchik commented

2023-08-02 14:31:17 +00:00

Can we use errors.Is here?

Can we use `errors.Is` here?

ale64bit commented

2023-08-02 14:59:31 +00:00

No. errors.Is checks equality. errors.As checks assignability.

type myError struct {
	data string
}

func (*myError) Error() string { return "my_error" }

func main() {
	{
		err := &myError{}
		fmt.Println(errors.Is(err, err))                     // true
		fmt.Println(errors.Is(&myError{}, &myError{}))       // false
		fmt.Println(errors.Is(&myError{"a"}, &myError{"b"})) // false
	}
	{
		err1 := &myError{"a"}
		err2 := &myError{"b"}
		fmt.Println(errors.As(err1, &err1)) // true
		fmt.Println(errors.As(err1, &err2)) // true
	}
}

No. `errors.Is` checks equality. `errors.As` checks assignability. ```go type myError struct { data string } func (*myError) Error() string { return "my_error" } func main() { { err := &myError{} fmt.Println(errors.Is(err, err)) // true fmt.Println(errors.Is(&myError{}, &myError{})) // false fmt.Println(errors.Is(&myError{"a"}, &myError{"b"})) // false } { err1 := &myError{"a"} err2 := &myError{"b"} fmt.Println(errors.As(err1, &err1)) // true fmt.Println(errors.As(err1, &err2)) // true } } ```

fyrchik commented

2023-08-03 07:01:21 +00:00

Is can be overridden (https://cs.opensource.google/go/go/+/refs/tags/go1.20.7:src/errors/wrap.go;l=53 ) and the variable we use to compare can be shared), so it looks somewhat more optimal.
But I agree it needs some support in code.

`Is` can be overridden (https://cs.opensource.google/go/go/+/refs/tags/go1.20.7:src/errors/wrap.go;l=53 ) and the variable we use to compare can be shared), so it looks somewhat more optimal. But I agree it needs some support in code.