Emit BigInt literals in hexadecimal if shorter (#4165) #4166
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. |
| Function<String, String> normalizeBigInt = s -> | ||
| s.substring(0, s.length() - 1).toLowerCase() + s.substring(s.length() - 1); |
There was a problem hiding this comment.
What's the downside of skipping this normalization in the test cases?
There was a problem hiding this comment.
If we skip this normalization, then the tests are enforcing that the compiler outputs the hexadecimal in lowercase, which is not necessarily a part of the compiler spec. In js spec, the hexadecimal encoding can be upper or lowercase including the 'x' character of "0x". (The final 'n' has to be lowercase though)
For simplicity it can be skipped.
As an example, if the compiler has a pass for optimizing brotli-compressed size, it may decide to convert all characters to upper case where applicable. This test would break. This is hypothetical and just to demonstrate the over-specification of the tests if we don't normalize.
There was a problem hiding this comment.
My general inclination is to try to keep the testing infrastructure simpler, even if that means we need to update the test cases more frequently. I think this makes it easier to read and understand the tests.
There was a problem hiding this comment.
Sounds good, will update the tests shortly
There was a problem hiding this comment.
@blickly I updated the tests. Please have another look, thank you!
- Remove the normalization step, which enforces that the BigInt emitter outputs the hexadecimal encoding in lower case.
|
Thanks for the contribution! |
Emit BigInt literals in hexadecimal format if it leads to shorter code size. This is important for cryptographic libraries, especially in verifiable computation, which have long list of large constants.
The exact threshold after which hexadecimal is shorter is 10^17, however to keep the code simple and extensible, we calculate both the decimal and hexadecimal encodings and compare their sizes.While both
f = (x) => x.toString(16).length + 2andg = (x) => x.toString().lengthare monotone, the difference of two monotone functions is not generally monotone. The above comment is incorrect, though the code is correct as is.