fix(goelements): handle Hex default int/uint values #827
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
Note: I'm working on getting my username added to our CLA group. Will keep you posted : ) |
|
Update: the CLA check is passing now. |
gogen/goelements.go
Outdated
|
|
||
| // In some YANG modules the default value could be written as base 16. | ||
| base := 10 | ||
| if strings.HasPrefix(strings.ToLower(value), "0x") { |
There was a problem hiding this comment.
Reading https://datatracker.ietf.org/doc/html/rfc7950#section-9.2.1
I think this line doesn't need ToLower
There was a problem hiding this comment.
Thanks a lot for the comment and the reference link. This is fixed in the latest commit. I checked the documentation of strconv.ParseInt and strconv.ParseUint. It turns out that the standard library already supports base inference for hexadecimal, octal, and binary integer formats. It supports sign prefix handling as well. So a simpler solution would be specifying base 0 to enable base inference.
A remaining question is that, should binary integer format trigger an error return?
Thanks!
There was a problem hiding this comment.
I think we can keep it simple and simply handle octal and hex as in the RFC. There are other quirks in strconv.ParseInt (e.g. integer literals) that we may not necessarily want to support.
There was a problem hiding this comment.
Thanks for the comment. Agreed, I added a couple checks to make sure that base 2 values and underscores cannot be used in default values.
.. and add test cases for hex/octal default int/uint values. The Go standard library already handles base inference for hexadecimal, octal, binary, and base-10, with sign prefix support. There's no need to reimplement the parsing and conversion of these integer values. According to https://datatracker.ietf.org/doc/html/rfc7950#section-9.2.1 one remaining question is, should the `yangDefaultValueToGo` function return an error if the default value for an integer is in binary format?
gogen/goelements.go
Outdated
|
|
||
| // Check if the value is an empty string. | ||
| if len(value) == 0 { | ||
| return "", yang.Ynone, fmt.Errorf("default value conversion: empty value") | ||
| } | ||
|
|
||
| // When the default value of int/uint is not base-10, check if the base should be supported according to | ||
| // https://datatracker.ietf.org/doc/html/rfc7950#section-9.2.1 | ||
| // | ||
| // When the first character in the value is `0`, the value could be hexadecimal, octal or binary. While hexadecimal | ||
| // and octal values are supported (with an optional sign prefix), binary is not allowed by the RFC. The format support | ||
| // of `strconv.ParseInt/ParseUint` functions should be further constrained. | ||
| if value[0] == '0' { | ||
| if len(value) > 1 && (value[1] == 'b' || value[1] == 'B') { | ||
| return "", yang.Ynone, fmt.Errorf("default value conversion: base 2 value `%s` is not allowed", value) | ||
| } | ||
| } | ||
|
|
||
| // Underscores are not allowed in the value, although `strconv` allows them in certain formats. | ||
| if strings.Contains(value, "_") { | ||
| return "", yang.Ynone, fmt.Errorf("default value conversion: `_` is not allowed in the value %s", value) | ||
| } | ||
|
|
There was a problem hiding this comment.
Instead of checking for negative cases, it might be cleaner to check for positive cases and then use ParseInt's error handling to deal with all other unexpected cases. So I'm thinking replacing this highlighted block with
base := 10
switch {
case strings.HasPrefix(value, "0"):
base = 8
case strings.HasPrefix(value, "0x"):
base = 16
value = strings.TrimLeft(value, "0x")
}There was a problem hiding this comment.
The RFC allows use of an optional +/- sign prefix for octal and hexadecimal values, so it will need a few more lines to handle all cases... In fact the previous commit needs to handle the binary negative case with a possible sign prefix too. Personally I'd prefer adding the sign handling to the previous commit's negative case handling. But let me know your thoughts. Thanks.
There was a problem hiding this comment.
Thanks Jay, and sorry for the delay in review. I'm open to keeping the current PR but I'm wondering how difficult it is to check the positive cases?
if strings.HasPrefix(value, "+") {
value = value[1:]
}
base := 10
switch {
case strings.HasPrefix(value, "0"), strings.HasPrefix(value, "-0"):
base = 8
case strings.HasPrefix(value, "0x"), ...:
base = 16
value = strings.TrimLeft(value, "0x")
}This looks shorter than the negative case handling and is more understandable. Are there other cases which I've missed here?
There was a problem hiding this comment.
Thanks! This should work similarly : ) But I think it still needs a couple more lines to return an error when seeing values with underscores.
There was a problem hiding this comment.
The binary format 0b needs to be checked before the switch statement too. Otherwise, the first switch case will let 0b pass through and give it base 8.
There was a problem hiding this comment.
I see, even the current code is not handling the underscores. I think this looks fine to me, thanks!
The tests didn't catch this typo because `ParseUint` returned an error on the sign prefix. But this should be fixed and the expected error should be `binary format is disallowed`.
This PR can help handle the issue described in #790 . Please let me know if there are comments or changes you'd suggest. Thanks.