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

Refactor TLA support #397

Merged
merged 1 commit into from
Jun 7, 2024
Merged

Refactor TLA support #397

merged 1 commit into from
Jun 7, 2024

Conversation

QuiiBz
Copy link
Contributor

@QuiiBz QuiiBz commented Apr 22, 2024

Closes #339

392bbff essentially reverts #6 and the next commits apply progressively the commits mentioned in #339.

@QuiiBz QuiiBz marked this pull request as ready for review April 22, 2024 10:22
@saghul
Copy link
Contributor

saghul commented Apr 22, 2024

Thank you for doing this! ❤️

@QuiiBz
Copy link
Contributor Author

QuiiBz commented Apr 22, 2024

I'm planning to use QuickJS/QuickJS-NG soon, so contributing to this project makes sense!

The only thing not implemented is TLA support in the REPL. The code from this fork seems quite different at a first glance, but I'll try to make it work soon. Not sure if that's a blocker for this PR.

@saghul
Copy link
Contributor

saghul commented Apr 22, 2024

I think not but I'll take a closer look. Thanks again!

@saghul
Copy link
Contributor

saghul commented Apr 25, 2024

The only 2 missing ones are:

bellard/quickjs@e44b793
bellard/quickjs@00967aa

I think we should at least include bellard/quickjs@00967aa here and the bits in bellard/quickjs@e44b793 that add the new eval flag. The REPL changes can be a subsequent PR.

@QuiiBz
Copy link
Contributor Author

QuiiBz commented Apr 25, 2024

Just added os.sleepAsync, the other changes in those two commits were already there (except the REPL changes).

@saghul saghul requested a review from chqrlie April 25, 2024 10:25
Copy link
Contributor

@saghul saghul left a comment

Choose a reason for hiding this comment

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

LGTM. @chqrlie can you PTAL too?

@chqrlie
Copy link
Collaborator

chqrlie commented May 1, 2024

LGTM. @chqrlie can you PTAL too?

Looking at this... Takes some time to ensure no unseen regressions, should be done soon.

run-test262.c Show resolved Hide resolved
@saghul
Copy link
Contributor

saghul commented May 27, 2024

Alright, I rebased this and added the missing REPL bits. @chqrlie PTAL!

Something I noticed is the difference in return values now:

qjs > os.readdir('.')
{
  value: [
    [
      '.',                   '..',                  'libregexp.c',
      'v8.sh',               'repl.js',             'test262.conf',
      'getopt_compat.h',     'quickjs.c',           'CMakeLists.txt',
      'LICENSE',             'run-test262.c',       'Makefile',
      'v8.txt',              'libunicode-table.h',  'qjs.c',
      'quickjs-c-atomics.h', 'libunicode.c',        'tests',
      'libbf.h',             'dirent_compat.h',     'cutils.c',
      'quickjs-libc.c',      'qjsc.c',              'gen',
      '.gitmodules',         'v8-tweak.js',         'README.md',
      'libregexp-opcode.h',  'list.h',              'a.out',
      '.gitignore',          'quickjs-opcode.h',    'libregexp.h',
      'examples',            't.js',                '.github',
      'quickjs-atom.h',      'doc',                 'quickjs.h',
      'unicode_gen_def.h',   'libunicode.h',        'v8.js',
      'test262_errors.txt',  'test262-fast.conf',   'build',
      'test262',             '.git',                'quickjs-libc.h',
      't.c',                 'unicode_download.sh', 'cutils.h',
      'unicode_gen.c',       'libbf.c'
    ],
    0
  ]
}
qjs >

vs before:

qjs > os.readdir('.')
[
  [
    '.',                   '..',                  'libregexp.c',
    'v8.sh',               'repl.js',             'test262.conf',
    'getopt_compat.h',     'quickjs.c',           'CMakeLists.txt',
    'LICENSE',             'run-test262.c',       'Makefile',
    'v8.txt',              'libunicode-table.h',  'qjs.c',
    'quickjs-c-atomics.h', 'libunicode.c',        'tests',
    'libbf.h',             'dirent_compat.h',     'cutils.c',
    'quickjs-libc.c',      'qjsc.c',              'gen',
    '.gitmodules',         'v8-tweak.js',         'README.md',
    'libregexp-opcode.h',  'list.h',              'a.out',
    '.gitignore',          'quickjs-opcode.h',    'libregexp.h',
    'examples',            't.js',                '.github',
    'quickjs-atom.h',      'doc',                 'quickjs.h',
    'unicode_gen_def.h',   'libunicode.h',        'v8.js',
    'test262_errors.txt',  'test262-fast.conf',   'build',
    'test262',             '.git',                'quickjs-libc.h',
    't.c',                 'unicode_download.sh', 'cutils.h',
    'unicode_gen.c',       'libbf.c'
  ],
  0
]
qjs >

Upstream is correct so it seems something is amiss here.

@saghul
Copy link
Contributor

saghul commented May 27, 2024

Simpler case:

qjs > var x;
{ value: undefined }

@saghul
Copy link
Contributor

saghul commented May 27, 2024

Ah bellard/quickjs@00967aa

@saghul
Copy link
Contributor

saghul commented May 27, 2024

Alright, this is now ready to do AFAICT! @chqrlie can you PTAL? I'd like to cut a release after this goes in.

@chqrlie
Copy link
Collaborator

chqrlie commented May 30, 2024

Alright, this is now ready to do AFAICT! @chqrlie can you PTAL? I'd like to cut a release after this goes in.

Very busy at the moment, will try to review by Friday night.

@saghul
Copy link
Contributor

saghul commented Jun 6, 2024

Gentle ping @chqrlie :-)

@saghul
Copy link
Contributor

saghul commented Jun 7, 2024

So, our implementation matches upstream now, 262 passes and I also tested with txiki.js: saghul/txiki.js#540

I'm going to merge this and if we need to make tweaks to it we can dot hat as we do any other bugfix.

@saghul saghul merged commit d3da56b into quickjs-ng:master Jun 7, 2024
48 checks passed
@chqrlie
Copy link
Collaborator

chqrlie commented Jun 7, 2024

So, our implementation matches upstream now, 262 passes and I also tested with txiki.js: saghul/txiki.js#540

I'm going to merge this and if we need to make tweaks to it we can dot hat as we do any other bugfix.

OK for me, I was about to write the same thing :)

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.

Redo TLA support by taking it from bellard/quickjs
3 participants