common: Improve IsHex and IsHexAddress & add tests

[stevenroose]

Fixes #15550.

[fjl]

The only use of IsHex is in IsHexAddress, also in package common. I think it would be best to delete IsHex and improve IsHexAddress instead. Bonus points if you can do it without a regular expression ;)

[stevenroose]

I have no experience with regular expressions in Go performance-wise. Are they bad? I will try. An address without the `0x` prefix should also be considered valid, right?

…

On 27 Nov 2017 15:34, "Felix Lange" ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In common/bytes.go <#15551 (comment)> : > func IsHex(str string) bool { - l := len(str) - return l >= 4 && l%2 == 0 && str[0:2] == "0x" + return hexRegex.MatchString(str) } The only use of IsHex is in IsHexAddress, also in package common. I think it would be best to delete IsHex and improve IsHexAddress instead. Bonus points if you can do it without a regular expression ;) — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#15551 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA0F3Gj9RGtLEayV5YZuIqf-yw3grX2qks5s6shqgaJpZM4QpsoN> .

[fjl]

Yes

[fjl]

Regular expressions aren't bad, but package regexp is big and it's nice to avoid using it whenever possible.

[stevenroose]

$ grep -rn "\"regexp\""
accounts/abi/bind/bind.go:26:	"regexp"
accounts/abi/type.go:22:	"regexp"
Binary file build/_workspace/pkg/linux_amd64/github.com/ethereum/go-ethereum/vendor/golang.org/x/tools/imports.a matches
build/update-license.go:31:	"regexp"
build/ci.go:56:	"regexp"
cmd/faucet/faucet.go:38:	"regexp"
common/compiler/solidity.go:27:	"regexp"
common/format.go:21:	"regexp"
common/bytes.go:22:	"regexp"
console/console.go:26:	"regexp"
internal/cmdtest/test_cmd.go:27:	"regexp"
p2p/discover/node.go:29:	"regexp"
p2p/discv5/node.go:29:	"regexp"
p2p/simulations/adapters/exec.go:32:	"regexp"
tests/init_test.go:27:	"regexp"
swarm/api/api.go:23:	"regexp"
vendor/github.com/davecgh/go-spew/spew/dump.go:26:	"regexp"
vendor/github.com/gizak/termui/helper.go:8:	"regexp"
vendor/github.com/gizak/termui/textbuilder.go:8:	"regexp"
vendor/github.com/huin/goupnp/httpu/serve.go:9:	"regexp"
vendor/github.com/huin/goupnp/soap/types.go:9:	"regexp"
vendor/github.com/huin/goupnp/ssdp/registry.go:8:	"regexp"
vendor/github.com/mattn/go-runewidth/runewidth_posix.go:7:	"regexp"
vendor/github.com/robertkrimen/otto/builtin_function.go:5:	"regexp"
vendor/github.com/robertkrimen/otto/cmpl_parse.go:5:	"regexp"
vendor/github.com/robertkrimen/otto/dbg/dbg.go:65:	"regexp"
vendor/github.com/robertkrimen/otto/otto.go:139:Go translates JavaScript-style regular expressions into something that is "regexp" compatible via `parser.TransformRegExp`.
vendor/github.com/robertkrimen/otto/otto_.go:5:	"regexp"
vendor/github.com/robertkrimen/otto/parser/README.markdown:79:TransformRegExp transforms a JavaScript pattern into a Go "regexp" pattern.
vendor/github.com/robertkrimen/otto/parser/expression.go:4:	"regexp"
vendor/github.com/robertkrimen/otto/parser/lexer.go:7:	"regexp"
vendor/github.com/robertkrimen/otto/parser/regexp.go:23:// TransformRegExp transforms a JavaScript pattern into  a Go "regexp" pattern.
vendor/github.com/robertkrimen/otto/type_date.go:6:	"regexp"
vendor/github.com/robertkrimen/otto/type_regexp.go:5:	"regexp"
vendor/github.com/robertkrimen/otto/value_string.go:6:	"regexp"
vendor/github.com/robertkrimen/otto/README.markdown:176:"regexp" compatible via `parser.TransformRegExp`. Unfortunately, RegExp requires
vendor/github.com/robertkrimen/otto/builtin.go:7:	"regexp"
vendor/github.com/robertkrimen/otto/builtin_string.go:5:	"regexp"
vendor/github.com/robertkrimen/otto/value_number.go:6:	"regexp"
vendor/github.com/Azure/go-autorest/autorest/date/time.go:4:	"regexp"
vendor/github.com/maruel/panicparse/stack/stack.go:21:	"regexp"
vendor/github.com/olekukonko/tablewriter/table.go:15:	"regexp"
vendor/github.com/olekukonko/tablewriter/util.go:12:	"regexp"
vendor/github.com/stretchr/testify/assert/assertions.go:11:	"regexp"
vendor/golang.org/x/text/language/maketables.go:21:	"regexp"
vendor/golang.org/x/tools/imports/mkstdlib.go:19:	"regexp"
vendor/golang.org/x/tools/imports/zstdlib.go:2846:	"regexp.Compile":                "regexp",
vendor/golang.org/x/tools/imports/zstdlib.go:2847:	"regexp.CompilePOSIX":           "regexp",
vendor/golang.org/x/tools/imports/zstdlib.go:2848:	"regexp.Match":                  "regexp",
vendor/golang.org/x/tools/imports/zstdlib.go:2849:	"regexp.MatchReader":            "regexp",
vendor/golang.org/x/tools/imports/zstdlib.go:2850:	"regexp.MatchString":            "regexp",
vendor/golang.org/x/tools/imports/zstdlib.go:2851:	"regexp.MustCompile":            "regexp",
vendor/golang.org/x/tools/imports/zstdlib.go:2852:	"regexp.MustCompilePOSIX":       "regexp",
vendor/golang.org/x/tools/imports/zstdlib.go:2853:	"regexp.QuoteMeta":              "regexp",
vendor/golang.org/x/tools/imports/zstdlib.go:2854:	"regexp.Regexp":                 "regexp",
vendor/golang.org/x/tools/imports/imports.go:21:	"regexp"
vendor/gopkg.in/check.v1/check.go:19:	"regexp"
vendor/gopkg.in/check.v1/checkers.go:6:	"regexp"
ethstats/ethstats.go:27:	"regexp"
log/handler_glog.go:22:	"regexp"

[fjl]

Hmm, so what this tells me is that we're not that far away from removing the common -> regexp dependency ;)

[stevenroose]

@fjl Updated. Also improved IsHexAddress and added some tests for that.

[stevenroose]

(That switch statement there is copied from Go's hex package.

[fjl]

Please remove blank lines in this function.

[stevenroose]

done

[fjl]

Please use for _, c := range []byte(src) because src is otherwise unused.

[fjl]

Please use a boolean expression instead of switch.

[stevenroose]

The switch is taken from the hex package. I think it's more readable. If you insist, I could change it...

[fjl]

This could be s = s[2:] to avoid repeating the check expression.

[stevenroose]

True.

[holiman]

This changes the semantics of IsHex/isHex. Previously, it expected a 0x-prefixed string, now it expects no prefix. That's fine, though, since it's internal we know it's not used anywhere where it will break anything.

[fjl]

The only use is in IsHexAddress, which already checks for the prefix ;)

[stevenroose]

Great!