Move unused noopError to test. Simplify handleNoOp function

[MarkKremer]

noopError wasn't used so I moved it to the test.

The nil check is already handled by the type assertion so I removed it.

[coveralls]

Coverage decreased (-50.9%) to 22.149% when pulling 7c70ac7e869b068c40b3f2f858b9cd22283aaaa1 on MarkKremer:markkremer_redundant_noop_impl into c548eed1757bd92440cf4a360b2ea8e62f72ebde on gorgonia:master.

[MarkKremer]

I very recently discovered that having an if statement in place that helps avoid doing type assertions can actually make things faster.

I made a stand-alone bench:

package test

import (
    "github.com/pkg/errors"
    "runtime"
    "testing"
)

// NoOpError is a useful for operations that have no op.
type NoOpError interface {
    NoOp() bool
}

type noopError struct{}

func (e noopError) NoOp() bool    { return true }
func (e noopError) Error() string { return "NoOp" }

func handleNoOpOld(err error) error {
    if err == nil {
        return nil
    }

    if _, ok := err.(NoOpError); !ok {
        return err
    }
    return nil
}

func handleNoOpNew(err error) error {
    if _, ok := err.(NoOpError); ok {
        return nil
    }

    return err
}

func Benchmark_handleNoOp(b *testing.B) {
    b.Run("old - nil", func(b *testing.B) {
        for i := 0; i < b.N; i++ {
            runtime.KeepAlive(handleNoOpOld(nil))
        }
    })
    b.Run("old - err", func(b *testing.B) {
        err := errors.New("")
        for i := 0; i < b.N; i++ {
            runtime.KeepAlive(handleNoOpOld(err))
        }
    })
    b.Run("old - noop", func(b *testing.B) {
        err := noopError{}
        for i := 0; i < b.N; i++ {
            runtime.KeepAlive(handleNoOpOld(err))
        }
    })

    b.Run("new - nil", func(b *testing.B) {
        for i := 0; i < b.N; i++ {
            runtime.KeepAlive(handleNoOpNew(nil))
        }
    })
    b.Run("new - err", func(b *testing.B) {
        err := errors.New("")
        for i := 0; i < b.N; i++ {
            runtime.KeepAlive(handleNoOpNew(err))
        }
    })
    b.Run("new - noop", func(b *testing.B) {
        err := noopError{}
        for i := 0; i < b.N; i++ {
            runtime.KeepAlive(handleNoOpNew(err))
        }
    })
}

Result:

Benchmark_handleNoOp/old_-_nil-8                1000000000               0.3128 ns/op
Benchmark_handleNoOp/old_-_err-8                84843207                14.12 ns/op
Benchmark_handleNoOp/old_-_noop-8               102648885               11.70 ns/op
Benchmark_handleNoOp/new_-_nil-8                354811396                3.369 ns/op
Benchmark_handleNoOp/new_-_err-8                80803771                14.09 ns/op
Benchmark_handleNoOp/new_-_noop-8               102191506               11.78 ns/op

nil will probably be the most common value by far. So that if statement does safe some time.

I'll change my PR to keep the nil check in place but still move the noopError struct.

[MarkKremer]

I think I just tanked the coverage by adding a test to a previously untested subpackage. :sweat_smile: I'm not sure what to do with this.

[dcu]

I think we should just merge I'm that case and that's the new baseline

[chewxy]

don't worry that much about coverage

[chewxy]

Can I merge this @MarkKremer

[MarkKremer]

Yes