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.

Sign up for GitHub

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

Jump to bottom

Emit BigInt literals in hexadecimal if shorter (#4165) #4166

Merged
merged 3 commits into from
May 30, 2024
Merged

Emit BigInt literals in hexadecimal if shorter (#4165) #4166

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3ae8acd
Emit BigInt literals in hexadecimal if shorter (#4165)
KimlikDAO-bot May 25, 2024
6e5ce38
Add tests for BigInt emitter (#4165)
KimlikDAO-bot May 28, 2024
7cc08dc
Simplify BigInt emitter tests
KimlikDAO-bot May 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/com/google/javascript/jscomp/CodeConsumer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@

import com.google.errorprone.annotations.ForOverride;
import com.google.javascript.rhino.Node;
import java.math.BigInteger;

/**
* Abstracted consumer of the CodeGenerator output.
Expand Down Expand Up @@ -367,6 +368,12 @@ void addNumber(double x, Node n) {
}
}

void addBigInt(BigInteger bi) {
String hexEncoded = "0x" + bi.toString(16) + "n";
String decimalEncoded = bi.toString() + "n";
addConstant(hexEncoded.length() < decimalEncoded.length() ? hexEncoded : decimalEncoded);
}

void addConstant(String newcode) {
add(newcode);
}
Expand Down
2 changes: 1 addition & 1 deletion src/com/google/javascript/jscomp/CodeGenerator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -418,7 +418,7 @@ protected void add(Node node, Context context, boolean printComments) {

case BIGINT:
Preconditions.checkState(childCount == 0, node);
cc.add(node.getBigInt() + "n");
cc.addBigInt(node.getBigInt());
break;

case TYPEOF:
Expand Down
34 changes: 34 additions & 0 deletions test/com/google/javascript/jscomp/CodePrinterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import com.google.javascript.rhino.Token;
import java.util.ArrayList;
import java.util.List;
import java.util.function.Function;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
Expand All @@ -47,6 +48,39 @@ public void testBigInt() {
assertPrint("-0b110n", "-6n");
assertPrint("-0o7n", "-7n");
assertPrint("-0x8n", "-8n");
assertPrintSame("1000000000n");
assertPrintSame("-10000000000n");

// Normalize the results to lowercase so the tests do not enfore the output is lowercase but
// instead the capitalization is left to the compiler.
Function<String, String> normalizeBigInt = s ->
s.substring(0, s.length() - 1).toLowerCase() + s.substring(s.length() - 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the downside of skipping this normalization in the test cases?

Copy link
Contributor Author

@KimlikDAO-bot KimlikDAO-bot May 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, will update the tests shortly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@blickly I updated the tests. Please have another look, thank you!

assertPrintAfterNormalization(
"7182582809266874004429817360811601465721521679690913024666851123813995272999n",
"0xfe132a356ec5f9279946c8fdf29780abd0387a8277e811069d174660b385b27n",
normalizeBigInt);
assertPrintAfterNormalization(
"0x000000fe132a356ec5f9279946c8fdf29780abd0387a8277e811069d174660b385b27n",
"0xfe132a356ec5f9279946c8fdf29780abd0387a8277e811069d174660b385b27n",
normalizeBigInt);
assertPrintAfterNormalization(
"-7182582809266874004429817360811601465721521679690913024666851123813995272999n",
"-0xfe132a356ec5f9279946c8fdf29780abd0387a8277e811069d174660b385b27n",
normalizeBigInt);
assertPrintAfterNormalization(
"-0o00774114521526730576223631215443757451360052750070365011677201040647213506301316055447n",
"-0xfe132a356ec5f9279946c8fdf29780abd0387a8277e811069d174660b385b27n",
normalizeBigInt);
assertPrintAfterNormalization(
"0o774114521526730576223631215443757451360052750070365011677201040647213506301316055447n",
"0xfe132a356ec5f9279946c8fdf29780abd0387a8277e811069d174660b385b27n",
normalizeBigInt);
assertPrintAfterNormalization(
"0b111111100001001100101010001101010110111011000101111110010010011110011001010001101100100" +
"0111111011111001010010111100000001010101111010000001110000111101010000010011101111110" +
"10000001000100000110100111010001011101000110011000001011001110000101101100100111n",
"0xfe132a356ec5f9279946c8fdf29780abd0387a8277e811069d174660b385b27n",
normalizeBigInt);
}

@Test
Expand Down
12 changes: 9 additions & 3 deletions test/com/google/javascript/jscomp/CodePrinterTestBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import com.google.javascript.jscomp.parsing.Config;
import com.google.javascript.rhino.Node;
import java.nio.charset.Charset;
import java.util.function.Function;
import org.jspecify.nullness.Nullable;
import org.junit.Before;

Expand Down Expand Up @@ -166,10 +167,11 @@ void assertPrettyPrintNode(String expectedJs, Node ast) {
assertThat(prettyPrintNode(ast)).isEqualTo(expectedJs);
}

protected void assertPrint(String js, String expected) {
protected void assertPrintAfterNormalization(String js, String expected,
Function<String, String> normalize) {
parse(expected); // validate the expected string is valid JS
assertThat(
parsePrint(
normalize.apply(parsePrint(
js,
newCompilerOptions(
new CompilerOptionBuilder() {
Expand All @@ -179,10 +181,14 @@ void setOptions(CompilerOptions options) {
options.setLineLengthThreshold(
CompilerOptions.DEFAULT_LINE_LENGTH_THRESHOLD);
}
})))
}))))
.isEqualTo(expected);
}

protected void assertPrint(String js, String expected) {
assertPrintAfterNormalization(js, expected, Function.identity());
}

protected void assertPrintSame(String js) {
assertPrint(js, js);
}
Expand Down