Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

opt: encoder support Uint64ToString #723

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

period331
Copy link

@period331 period331 commented Dec 13, 2024

有很多时候输出给前端的数据,包含uint64类型时,在js里会丢失精度;
且下游传递过来的数据结构不可枚举,所以将给前端的数据中,统一将uint64转成字符串

@CLAassistant
Copy link

CLAassistant commented Dec 13, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Collaborator

@AsterDY AsterDY left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

你这个只支持了非x86下的实现,x86需要在encoder/x86中实现哈

api.go Outdated

// Int64 or Uint64 into strings on Marshal
Int64ToString bool
Uint64ToString bool
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

两个选项没必要,合并成一个选项吧

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

收到

@AsterDY
Copy link
Collaborator

AsterDY commented Dec 16, 2024

统一用json.Number不行么?

@period331
Copy link
Author

统一用json.Number不行么?

目前看,不太行,下游传递回来的数据结构千奇百怪的,decode到一个map[string]any里;
然后策略可能在这个map的某个层级塞入各种类型的数据;

@period331 period331 changed the title add encoder support Int64ToString and Uint64ToString opt: add encoder support Uint64ToString Dec 16, 2024
@period331 period331 changed the title opt: add encoder support Uint64ToString opt: encoder support Uint64ToString Dec 16, 2024
@period331
Copy link
Author

糟了,go1.17,没有any,我改下

encode_test.go Outdated
NumberStr: "46",
},
want: `{
tests := []struct {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

不要用 tab 缩进,diff太多了

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

收到,这个是编辑器自动修改的格式

@@ -94,6 +94,9 @@ type Config struct {

// Encode Infinity or Nan float into `null`, instead of returning an error.
EncodeNullForInfOrNan bool

// Uint64 into strings on Marshal
Uint64ToString bool
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

为啥不需要Int64呢?我的意思是Int64和Uint64都用一个选项控制就行了

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😓😓,我们是PHP(c++扩展)迁移go的,老逻辑只处理了uint64,int64没处理。如果同一个选项控制,会有大量diff😅
所以我给分开了,10多年的老代码了,diff太多迁移成本剧增🤡

Copy link
Author

@period331 period331 Dec 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AsterDY 你看可以不?要么两个选项,要么只有一个uint64的选项,如果合并到一起,我们这边就用不了了😂😂😂

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

那就分开吧

@@ -179,7 +179,7 @@ func (self *Compiler) compileOps(p *ir.Program, sp int, vt reflect.Type) {
case reflect.Bool:
p.Add(ir.OP_bool)
case reflect.Int:
p.Add(ir.OP_int())
p.Add(ir.OP_int(), ir.OP_i)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里没看懂,应该没必要加这个CompactOp。在具体的 OP_i64读取flag bit或IsMapKey()进行处理就行了

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

因为vm里是用uint64的逻辑处理int的,如果没有compatOp,则会把int也处理

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

底层的OP int和uint都是分开的,这里肯定是不需要的。“vm里是用uint64的逻辑处理int的”
vm代码里面明明是分开的,不知道你指的是啥

Copy link
Author

@period331 period331 Dec 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AsterDY

底层的OP int和uint都是分开的

vm里确实是分开的,但是vm里Add时,调用了这样一个函数:

func OP_int() Op {
	switch _INT_SIZE {
	case 32:
		return OP_i32
	case 64:
		return OP_i64
	default:
		panic("unsupported int size")
	}
}

uint也有类似处理,这相当于int用int64、uint用uint64的相关函数来处理了
如果没有CompatOp的话,在int64/uint64的相关处理函数内,就不知道处理的是int/uint了

还有别的地方做差异化处理了吗?

@@ -860,8 +860,21 @@ func (self *Assembler) _asm_OP_u32(_ *ir.Instr) {
self.store_int(16, _F_u64toa, "MOVLQZX")
}

func (self *Assembler) _asm_OP_u64(_ *ir.Instr) {
func (self *Assembler) _asm_OP_u64(i *ir.Instr) {
if i.CompatOp() == ir.OP_i || i.IsMapKey() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

同上,没看懂i.CompatOp() == ir.OP_i的意义是什么。而且和vm的实现逻辑也不一致

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

同样的,如果没有compatOp,则会把int也处理

Copy link
Author

@period331 period331 Dec 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

而且和vm的实现逻辑也不一致

实现是一致的,只是判断方式不同。

不过也有可能是我不太会asm开发,完全是临时抱佛脚,参考_asm_OP_empty_obj函数写的

Copy link
Author

@period331 period331 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

再发起一下评审,关于Int64和Uint64的选项问题,辛苦给予答复,谢谢啦

@period331
Copy link
Author

@AsterDY 另外我看1.23.4版本中,decode的测试有报错,但看着和我的变更无关;可以帮忙看下吗?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants