common: Improve IsHex and IsHexAddress & add tests #15551

stevenroose · 2017-11-24T11:23:22Z

fjl · 2017-11-27T14:33:38Z

common/bytes.go

 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 ;)

stevenroose · 2017-11-27T15:27:23Z

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 · 2017-11-27T15:28:37Z

Yes

fjl · 2017-11-27T15:29:18Z

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

stevenroose · 2017-11-27T16:12:51Z

$ 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 · 2017-11-27T21:11:10Z

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

stevenroose · 2017-11-29T12:10:14Z

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

stevenroose · 2017-11-29T12:10:40Z

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

fjl · 2017-12-01T12:55:57Z

common/bytes.go

+		}
+	}
+
+	return true
 }


Please remove blank lines in this function.

fjl · 2017-12-01T12:55:57Z

common/bytes.go

+	}
+
+	src := []byte(str)
+	for _, c := range src {


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

fjl · 2017-12-01T12:55:57Z

common/bytes.go

+	case 'A' <= c && c <= 'F':
+		return true
+	}
+	return false


Please use a boolean expression instead of switch.

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

fjl · 2017-12-01T12:55:57Z

common/types.go

-	if len(s) == 2+2*AddressLength && IsHex(s) {
-		return true
+	if HasHexPrefix(s) {
+		return len(s) == 2+2*AddressLength && IsHex(s)


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

Also unexport IsHex, HasHexPrefix because IsHexAddress is the only caller.

holiman · 2017-12-04T11:15:21Z

common/bytes.go

+	return ('0' <= c && c <= '9') || ('a' <= c && c <= 'f') || ('A' <= c && c <= 'F')
+}
+
+func isHex(str string) bool {


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.

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

stevenroose · 2017-12-05T16:26:17Z

Great!

fjl reviewed Nov 27, 2017

View reviewed changes

stevenroose force-pushed the ishex branch 2 times, most recently from 7497bf2 to 8360ce3 Compare November 29, 2017 12:09

stevenroose force-pushed the ishex branch 2 times, most recently from 643f3cb to 25abdcb Compare November 29, 2017 12:35

stevenroose changed the title ~~common: Correct IsHex to check if content is hex~~ common: Improve IsHex and IsHexAddress & add tests Nov 29, 2017

fjl requested changes Dec 1, 2017

View reviewed changes

common: Improve IsHex and IsHexAddress & add tests

84d6d87

stevenroose force-pushed the ishex branch from 25abdcb to 84d6d87 Compare December 2, 2017 21:47

common: fixup IsHexAddress

1ce722c

Also unexport IsHex, HasHexPrefix because IsHexAddress is the only caller.

handrijunaidi007 approved these changes Dec 4, 2017

View reviewed changes

holiman approved these changes Dec 4, 2017

View reviewed changes

common: fix vet

48bbf26

fjl merged commit afb8154 into ethereum:master Dec 4, 2017

karalabe added this to the 1.8.0 milestone Dec 14, 2017

5chdn mentioned this pull request Jan 30, 2018

Merge go-ethereum updates into 2.5.4 Musicoin/go-musicoin#77

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

common: Improve IsHex and IsHexAddress & add tests #15551

common: Improve IsHex and IsHexAddress & add tests #15551

stevenroose commented Nov 24, 2017

fjl Nov 27, 2017

stevenroose commented Nov 27, 2017 via email

fjl commented Nov 27, 2017

fjl commented Nov 27, 2017

stevenroose commented Nov 27, 2017

fjl commented Nov 27, 2017

stevenroose commented Nov 29, 2017

stevenroose commented Nov 29, 2017

fjl Dec 1, 2017

stevenroose Dec 2, 2017

fjl Dec 1, 2017

fjl Dec 1, 2017

stevenroose Dec 2, 2017

fjl Dec 1, 2017

stevenroose Dec 2, 2017

holiman Dec 4, 2017

fjl Dec 4, 2017

stevenroose commented Dec 5, 2017

common: Improve IsHex and IsHexAddress & add tests #15551

common: Improve IsHex and IsHexAddress & add tests #15551

Conversation

stevenroose commented Nov 24, 2017

Choose a reason for hiding this comment

stevenroose commented Nov 27, 2017 via email

fjl commented Nov 27, 2017

fjl commented Nov 27, 2017

stevenroose commented Nov 27, 2017

fjl commented Nov 27, 2017

stevenroose commented Nov 29, 2017

stevenroose commented Nov 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevenroose commented Dec 5, 2017