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.

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

When dumping icode, include string constants #1693

Merged
merged 1 commit into from
Oct 12, 2024

Conversation

andreabergia
Copy link
Contributor

When Token.printICode is set to true (generally only for debugging purposes), Rhino prints all the interpreter bytecode it generated. For the Icode_REG_STRxx instructions Rhino will also print inline the string value, but it does not do it for the (far more common) Icode_REG_STR_Cxx instructions.

This tiny PR just extends the same behavior to the Icode_REG_STR_Cxx. Therfore, for this js code:

a + b

it will now print this:

ICode dump, for null, length = 10
MaxStack = 2
 [0] LINE : 2
 [3] REG_STR_C0 "a"
 [4] NAME
 [5] REG_STR_C1 "b"
 [6] NAME
 [7] ADD
 [8] POP_RESULT
 [9] RETURN_RESULT

whereas the old version would have just printed something like:

 [3] REG_STR_C0

There are no unit tests for all this stuff, but it's currently being used only for debugging purposes... so I didn't really think that it was worth creating a whole test suite from scratch for this change.

@rbri
Copy link
Collaborator

rbri commented Oct 11, 2024

@andreabergia build fails because of spotless...

@andreabergia andreabergia force-pushed the icode-dump-string-constants branch from f9907ba to f28f066 Compare October 11, 2024 12:27
@andreabergia
Copy link
Contributor Author

@andreabergia build fails because of spotless...

🤦 I'll learn... eventually

@gbrail
Copy link
Collaborator

gbrail commented Oct 12, 2024

Seems very reasonable to me. Thanks!

@gbrail gbrail merged commit 3199d9c into mozilla:master Oct 12, 2024
1 check passed
@andreabergia andreabergia deleted the icode-dump-string-constants branch October 14, 2024 07:44
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.

3 participants