Skip to content

Conversation

@brk0v
Copy link

@brk0v brk0v commented Jan 15, 2020

No description provided.

bytes.go Outdated
return err
}
case int, int8, int16, int32, int64:
*b = Base2Bytes(reflect.ValueOf(v).Int())

Choose a reason for hiding this comment

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

I think it can fail with an overflow error if v > MaxInt32. Could you please add a test for int64 value that is > max int32

Copy link
Owner

Choose a reason for hiding this comment

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

Also I'd prefer not to use reflect unless necessary, which it doesn't appear to be.

Copy link
Author

Choose a reason for hiding this comment

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

Updated. Thank you for review.

Talking about overflow. The following code in parser checks for overflows:

			if e, ok := err.(*strconv.NumError); ok &&
				e.Err == strconv.ErrRange {

				p.panicf("Integer '%s' is out of the range of 64-bit "+
					"signed integers.", it.val)

https://github.com/BurntSushi/toml/blob/a368813c5e648fee92e5f6c30e3944ff9d5e8895/parse.go#L196

Also in TOML spec ints are always int64.

oldBytesUnitMap = MakeUnitMap("B", "B", 1024)
)

var ErrIncorrectType = errors.New("not a string or int64")

Choose a reason for hiding this comment

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

can be int, int8, int16, int32 as well

Copy link
Author

Choose a reason for hiding this comment

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

It can be only int64 or string.

TOML spec says that all ints are int64:

64 bit (signed long) range expected (−9,223,372,036,854,775,808 to 9,223,372,036,854,775,807).

https://github.com/toml-lang/toml#user-content-integer

Copy link

Choose a reason for hiding this comment

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

may be you should rename error? it is toml specific incorrect type error, but not package-wide incorrect type error.

@brk0v brk0v force-pushed the brk0v/toml-unmarshal branch from 3edde86 to 6a0232f Compare January 28, 2020 11:00
@hanjm
Copy link
Contributor

hanjm commented Sep 12, 2021

Hi, the Prometheus/Thanos use this lib, also need to deal with yaml. I found a problem the marshalled data can not be unmarshal.
I think implement encoding.TextMarshaler encoding.TextUnmarshaler would be better, it make json/ yaml / toml all the work fine. I create a MR #12, could you help me review, thank you. :)

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.

5 participants