random reflection improvements #3470
Conversation
|
This is in no way its final form; I'll pick and choose and construct better PRs, but this is my current WIP branch. |
aykevl
left a comment
aykevl
left a comment
There was a problem hiding this comment.
Looks very reasonable. Some early comments and suggestions below.
dgryski
force-pushed
the
dgryski/reflect-refactor-3
branch
2 times, most recently
from
2513ac8 to
23bc045
Compare
dgryski
force-pushed
the
dgryski/reflect-refactor-3
branch
2 times, most recently
from
807771c to
a0ff5c1
Compare
dgryski
force-pushed
the
dgryski/reflect-refactor-3
branch
from
65c31b6 to
41addd2
Compare
|
Bumping the |
dgryski
force-pushed
the
dgryski/reflect-refactor-3
branch
from
41addd2 to
305abe8
Compare
dgryski
force-pushed
the
dgryski/reflect-refactor-3
branch
from
32d1713 to
5c22c9d
Compare
aykevl
left a comment
aykevl
left a comment
There was a problem hiding this comment.
I reviewed most of the PR, but didn't review value.go completely. But figured it would be useful to send review feedback already.
Assuming this gets more of encoding/json to work, can you also add some tests to testdata/json.go? Just a few lines to prove the new features work (the testdata/* files are run on more platforms than the reflect package tests so might catch more bugs).
| c.getTypeCode(types.NewPointer(typ)), // ptrTo | ||
| c.getTypeCode(typ.Underlying()), // underlying | ||
| llvm.ConstInt(c.uintptrType, uint64(len(name)), false), // length | ||
| llvm.ConstArray(c.ctx.Int8Type(), buf), |
There was a problem hiding this comment.
I believe this has the same effect and may be a bit faster or more readable:
| llvm.ConstArray(c.ctx.Int8Type(), buf), | |
| llvm.ConstString(name, false), |
| typeFieldTypes = append(typeFieldTypes, | ||
| types.NewVar(token.NoPos, nil, "ptrTo", types.Typ[types.UnsafePointer]), | ||
| types.NewVar(token.NoPos, nil, "underlying", types.Typ[types.UnsafePointer]), | ||
| types.NewVar(token.NoPos, nil, "len", types.Typ[types.Uintptr]), | ||
| types.NewVar(token.NoPos, nil, "name", types.NewArray(types.Typ[types.Int8], int64(len(typ.Obj().Name())))), | ||
| ) |
There was a problem hiding this comment.
You are missing the corresponding update to the comment at the top of src/reflect/type.go.
| "libc": "picolibc", | ||
| "automatic-stack-size": true, | ||
| "default-stack-size": 2048, | ||
| "default-stack-size": 4096, |
There was a problem hiding this comment.
👍 for tests this is probably more appropriate.
| return Value{ | ||
| typecode: pointerTo(v.typecode), | ||
| value: unsafe.Pointer(&v.value), | ||
| flags: v.flags, |
There was a problem hiding this comment.
I think this is the bug you mentioned on Slack? Because I think you need to clear the indirect flag here:
| flags: v.flags, | |
| flags: v.flags &^ valueFlagIndirect, |
|
Closing in favour of #3486 (and others) |
3 participants
No description provided.