psanford / wormhole-william

End-to-end encrypted file transfer. A magic wormhole CLI and API in Go (golang).
MIT License
1.07k stars 54 forks source link

wormhole: Don't call reflect twice on message #94

Closed Jacalz closed 10 months ago

Jacalz commented 1 year ago

Cleans up some code that was doing double reflect calls when only one was necessary,

psanford commented 1 year ago

The go specification does not require an interface implementation to be a pointer type, as long as the method receiver is not a pointer:

package main

import (
    "fmt"
)

type concreteFoo struct {
}

func (c concreteFoo) String() string {
    return "concreteFoo"
}

type stringer interface {
    String() string
}

func takesAStringer(s stringer) {
    fmt.Println(s.String())
}

func main() {
    f := concreteFoo{}
    takesAStringer(f)
}
Jacalz commented 1 year ago

But the methods for the collectable types used within the project are all pointer receivers. image I don't see any reason for keeping the runtime check around for something that only can occur as a bug on the developer side and is checked by the compiler in our case as all types use pointer receivers (like is most usual in Go code over all).

psanford commented 1 year ago

I would take this change If you wrote a static analysis test that showed that every implementation of this interface used pointer types.

Jacalz commented 1 year ago

I've pushed a commit to at least not get the value twice from msg using reflect. It should be better than nothing in the meantime.

Jacalz commented 1 year ago

I might actually have a better fix for this. Will see what I can do.

Jacalz commented 1 year ago

I have opened https://github.com/psanford/wormhole-william/pull/95 with a different approach that checks the struct pointer in a different way without using the reflect package at all. If that PR is rejected, this is still a nice improvement over the previous code.

psanford commented 1 year ago

Can you squash this into a single commit?

Jacalz commented 1 year ago

Done

Jacalz commented 10 months ago

Ping @psanford