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

remove some obsolete code and add another test #1318

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rbri
Copy link
Collaborator

@rbri rbri commented Apr 10, 2023

Looks like this code was part of backward sync from HtmlUnit changes into Rhino.
But the code

  • produces different results when running with/without optimization
  • produces wrong results without optimization
  • is no longer needed from HtmlUnit's point of view
  • consumes some memory

see also HtmlUnit/htmlunit#354

@p-bakker
Copy link
Collaborator

p-bakker commented Apr 11, 2023

Hard time judging this PR as I'm unsure how the code removal in this PR affects behavior, I agree that we shouldn't have different behavior depending on the optLevel used.

However, it seems that the ECMAScript spec is not clearly defining how conditionally defined functions ought to behave (judging by https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/function#block-level_function_declaration), so which behavior do we want to follow?

The added test is running in non-strict (as in the test script doesn't have "use strict" in it). As the behavior in strict mode is clearly defined (function declarations inside blocks in strict-mode are local to the block, and hoisted within that block), shouldn't we have a test for strict-mode as well? (Not sure whether the MozillaSuiteTest already runs tests in both strict and non-strict mode, like the test262uite does)

And the code removed, isn't that code that is needed for a proper strict-mode implementation?

@rbri
Copy link
Collaborator Author

rbri commented Apr 12, 2023

@p-bakker you are right. the problem seems to be the scrict mode, working on an update.

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.

None yet

2 participants