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

Compute Token and Icode constants #1648

Merged
merged 1 commit into from
Sep 25, 2024

Conversation

rPraml
Copy link
Contributor

@rPraml rPraml commented Sep 24, 2024

This is an alternate approach for solving the problems mentioned here #1639

and should avoid large renumbering as you can see here: https://github.com/mozilla/rhino/pull/1593/files#diff-c1caeb9d13c1dceec5f4603f3836f3e227350ee1a57679238ce9a3a434601b60


// Stack: ... value2 value1 -> ... value2 value1 value2 value1
Icode_DUP2 = -2,
Icode_DUP2 = Icode_DUP - 1,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Value -3 is unused

REGEXP = SHNE + 1,
BINDNAME = REGEXP + 1,
THROW = BINDNAME + 1,
RETHROW = THROW + 1, // rethrow caught exception: catch (e if ) use it
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When inserting new opcodes, only the line before and after the insertion point has to be changed

@gbrail
Copy link
Collaborator

gbrail commented Sep 24, 2024

This is awfully clever -- I have not seen this before in Java and I'm wondering why not! I'll try it tonight.

@gbrail
Copy link
Collaborator

gbrail commented Sep 25, 2024

Does anyone else have a concern about this -- I'm having trouble seeing the downside and would like to merge it.

@p-bakker
Copy link
Collaborator

LGTM (we use the same approach in JS running in Rhino works like a charm 🙂)

@rbri
Copy link
Collaborator

rbri commented Sep 25, 2024

Does anyone else have a concern about this -- I'm having trouble seeing the downside and would like to merge it.

+1

We use this pattern since years for JDBC index based access of result sets (sometimes using the index is a bit faster)

@rPraml
Copy link
Contributor Author

rPraml commented Sep 25, 2024

I'm having trouble seeing the downside and would like to merge it.

The only downside is IMHO, you must understand the concept and add new constants the correct way (which should be more straight forward as before)
The produced class file should be the same, as the compiler resolves the constants at compile time.

@p-bakker
Copy link
Collaborator

p-bakker commented Sep 25, 2024

One slightly different approachwe also use and that is imho easier to understand and less likely to go wrong when adding new codes (at the cost of declaring another (private) variable):

private static int count = 0,

static final int
    // delete operator used on a name
    Icode_DELNAME = count--,

    // Stack: ... value1 -> ... value1 value1
    Icode_DUP = count--,

....

    // Call to GetTemplateLiteralCallSite
    Icode_TEMPLATE_LITERAL_CALLSITE = count--,
    Icode_LITERAL_KEYS = count--,
    Icode_LITERAL_KEY_SET = count--,
 
   MIN_CODE = count;

@rPraml
Copy link
Contributor Author

rPraml commented Sep 25, 2024

One slightly different approachwe also use and that is imho easier to understand and less likely to go wrong when adding new codes (at the cost of declaring another (private) variable):

private static int count = 0,

static final int
    // delete operator used on a name
    Icode_DELNAME = count--,
...

Unfortunately, this does not work here, as the result isn't computed at compile time, but on class instantiation. So these are no compile-time 'constants', that can be used in switch statements or annotations.

@p-bakker
Copy link
Collaborator

K, similar stuff in JavaScript dus do the trick, but Java ain't JavaScript or vise versa 🙂

@rbri rbri merged commit 1b11e69 into mozilla:master Sep 25, 2024
1 check passed
@rbri
Copy link
Collaborator

rbri commented Sep 25, 2024

@rPraml thanks a lot for this

@rbri rbri mentioned this pull request Sep 25, 2024
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.

4 participants