search

freeconf / yang

Standards-based management for Golang microservices
Apache License 2.0
38 stars 15 forks source link

Machine-dependent integer types are used for representing 32-bit yang integers #106

Closed Kirhchoff closed 7 months ago

Kirhchoff commented 7 months ago

YANG uint32 and int32 types are parsed as uint and int Golang types respectively. Those types are machine-dependent and can actually be 64-bit integers, depending on the architecture.

Machine-independent types uint32 and int32 should be used instead.

See: https://github.com/freeconf/yang/blob/ef92ddeb9f99e00a015beea651802b1ccd40b3d4/val/conv.go#L490 https://github.com/freeconf/yang/blob/ef92ddeb9f99e00a015beea651802b1ccd40b3d4/val/conv.go#L407

dhubler commented 7 months ago

ah, i wasn't aware. I support changing this FWIW. hopefully impact is minimal.

Kirhchoff commented 7 months ago

As far as I know, this change will break the API, because there is no implicit conversion between ints and int32s. So the applications that used this currently with int fields, will stop working and have to change their data structures unfortunately. It would be great to fix this, but it may have consequences :)

dhubler commented 7 months ago

There will be api changes, but maybe there is a simple convenience method we can create to adjust. I created a branch that handles the initial work we can use to investigate the impact.

https://github.com/freeconf/yang/pull/new/i-106-int32

BTW: Not saying now is the time, but I've wanted to take a second look at the conv module that still uses it's original design some 8 years ago and been source of many community patches.

Kirhchoff commented 7 months ago

Looks good to me. Just make sure the strconv.ParseUint usage in toInt32 function is correct. Shouldn't there be the strconv.ParseInt used instead?

dhubler commented 7 months ago

indeed, ParseInt, fixed and merged branch