extend stdlib to allow import of more packages #1099

cornelk · 2020-05-11T21:56:58Z

Closes #752

these changes allow a successful import of the following go 1.14 stdlib packages:

	_ "database/sql"
	_ "debug/elf"
	_ "debug/macho"
	_ "debug/pe"
	_ "debug/plan9obj"
	_ "html"
	_ "mime"
	_ "testing"

aykevl · 2020-05-12T15:53:29Z

This PR conflicts with #1012, which I think should be merged first as it also cleans up the os package a bit.

cornelk · 2020-05-13T08:06:11Z

@aykevl rebased now on current dev branch after #1012 got merged

src/os/errors.go

aykevl · 2020-05-15T14:21:07Z

src/os/errors.go

+)
+
+func IsPermission(err error) bool {
+	return err == ErrPermission


Not complete (it doesn't unwrap the error) but I guess it's fine for now.

src/reflect/type.go

aykevl · 2020-05-15T14:33:40Z

src/reflect/type.go

+// Lookup returns the value associated with key in the tag string.
+func (tag StructTag) Lookup(key string) (value string, ok bool) {


Get and Lookup will need tests. Please add them to testdata/reflect.go. You can probably just try to lookup a few tags and if they are present, print them (just like many other properties of types/values are printed).

I think it may be better to use something other than a string for StructTag eventually. The last time I looked through the standard library, no package was using StructTag as a string but instead just called the Get and Lookup functions. Therefore, I think it would be feasible to do the key/value splitting at compile time, avoiding the code size cost of doing it at runtime (and possibly speeding things up as well, although that's not the goal).

Are you sure you actually need Get and Lookup? You could perhaps simplify things by stubbing them out. At least when I tested the encoding/json package, I think I managed to do without for a proof-of-concept.

the StructTag type with it's functions is actively used in common libraries github.com/BurntSushi/toml and gopkg.in/yaml.v2

I added your proposal to do it at compile time as a TODO comment. For this MR i focused on getting more libraries to compile.

I checked both libraries.

github.com/BurntSushi/toml passes the .Tag field as a StructTag to getOptions and only uses the .Get function. That would allow doing the key/value parsing at compile time.

gopkg.in/yaml.v2 unfortunately does use the raw string. It looks like they fall back to using the whole string if the struct tag is not in a key/value format. I think they shouldn't do this as it breaks the convention, but it's probably hard to persuade them to change this (there is probably code that depends on this behavior).

Not sure what to do here, maybe using raw strings is better after all (even though it will likely increase code size). Maybe there are optimizations possible here, but I'm not sure whether that's possible in a sane way.

Did you write this code yourself or did you copy it? If you copied it, it should have a copyright note (just like the strconv.go file).

cornelk · 2020-05-21T12:22:49Z

@aykevl i updated the PR

deadprogram · 2020-05-28T06:52:44Z

@aykevl any further feedback? Seems like all of the comments have been addressed.

deadprogram · 2020-06-09T06:04:51Z

Reminder to @aykevl about this PR. LGTM.

aykevl

Thanks for this PR!
For the next time, please split it up a bit more so that the changes are a bit more focused. This helps with review and with investigating regressions (if there are any).

aykevl · 2020-06-16T22:19:12Z

src/reflect/strconv.go

+)
+
+// ErrSyntax indicates that a value does not have the right syntax for the target type.
+var ErrSyntax = badSyntax{}


This file adds a number of exported symbols. Because this is the reflect package, these (copied) symbols should not be exported.

aykevl · 2020-06-16T22:21:59Z

src/reflect/type.go

+// Lookup returns the value associated with key in the tag string.
+func (tag StructTag) Lookup(key string) (value string, ok bool) {


Did you write this code yourself or did you copy it? If you copied it, it should have a copyright note (just like the strconv.go file).

aykevl · 2020-06-16T22:26:31Z

src/runtime/mprof.go

+package runtime
+
+// Stack is a stub, not implemented
+func Stack(buf []byte, all bool) int {


You could add this to stack.go, which has similar functions (to avoid yet another small file).

None of these are likely to be implemented considering what TinyGo is designed for.

do not export strconv functions in reflect

cornelk · 2020-06-18T19:39:37Z

@aykevl comments are addressed, I will split it up next time.

deadprogram · 2020-06-23T09:56:03Z

Thank you very much for working on this contribution, @cornelk now that all of the comments have been addressed, I am going to squash/merge. Thanks again!

deadprogram · 2020-06-23T10:11:43Z

The tests previous passed, but now the CI fails for all Linux platforms. Perhaps rebasing again against dev would have been a good idea before I merged this. My bad.

We now need to fix this right away.

deadprogram · 2020-06-23T18:12:52Z

Thanks to @jaddr2line root problem was discovered (of course unrelated to this PR) and I just pushed a fix to the CircleCI build.

extend stdlib to allow import of more packages

f068eb6

aykevl reviewed May 15, 2020

View reviewed changes

cornelk added 5 commits May 21, 2020 13:52

Merge branch 'dev' into extend_stdlib

c3f7c28

os: move errors to separate file

8b46fa3

reflect: reimplement Unquote

ab9dc3d

add struct tag test

c5d1578

reflect: add TODO to do split at compile time

d865e6e

Merge branch 'dev' into extend_stdlib

5696b98

aykevl reviewed Jun 16, 2020

View reviewed changes

add copyright header

76d38a3

do not export strconv functions in reflect

deadprogram merged commit 720a54a into tinygo-org:dev Jun 23, 2020

niaow added this to the v0.14 milestone Jun 27, 2020

		// Lookup returns the value associated with key in the tag string.
		func (tag StructTag) Lookup(key string) (value string, ok bool) {

extend stdlib to allow import of more packages #1099

extend stdlib to allow import of more packages #1099

Uh oh!

Conversation

cornelk commented May 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aykevl commented May 12, 2020

Uh oh!

cornelk commented May 13, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cornelk commented May 21, 2020

Uh oh!

deadprogram commented May 28, 2020

Uh oh!

deadprogram commented Jun 9, 2020

Uh oh!

aykevl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cornelk commented Jun 18, 2020

Uh oh!

deadprogram commented Jun 23, 2020

Uh oh!

deadprogram commented Jun 23, 2020

Uh oh!

deadprogram commented Jun 23, 2020

Uh oh!

Uh oh!

cornelk commented May 11, 2020 •

edited

Loading