quickjs-ng / quickjs

QuickJS, the Next Generation: a mighty JavaScript engine
https://quickjs-ng.github.io/quickjs/
MIT License
972 stars 79 forks source link

Refactor TLA support #397

Closed QuiiBz closed 3 months ago

QuiiBz commented 5 months ago

Closes https://github.com/quickjs-ng/quickjs/issues/339

392bbff5470bc9a9fad81aa685c931353272752e essentially reverts https://github.com/quickjs-ng/quickjs/pull/6 and the next commits apply progressively the commits mentioned in https://github.com/quickjs-ng/quickjs/issues/339.

saghul commented 5 months ago

Thank you for doing this! ❤️

QuiiBz commented 5 months ago

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 commented 5 months ago

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

saghul commented 5 months ago

The only 2 missing ones are:

https://github.com/bellard/quickjs/commit/e44b793e3817766d9045ed2a776658fcbe0f2790 https://github.com/bellard/quickjs/commit/00967aac2408af0d3e591918b25e700229c9cdb3

I think we should at least include https://github.com/bellard/quickjs/commit/00967aac2408af0d3e591918b25e700229c9cdb3 here and the bits in https://github.com/bellard/quickjs/commit/e44b793e3817766d9045ed2a776658fcbe0f2790 that add the new eval flag. The REPL changes can be a subsequent PR.

QuiiBz commented 5 months ago

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

chqrlie commented 5 months ago

LGTM. @chqrlie can you PTAL too?

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

saghul commented 4 months ago

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 commented 4 months ago

Simpler case:

qjs > var x;
{ value: undefined }
saghul commented 4 months ago

Ah https://github.com/bellard/quickjs/commit/00967aac2408af0d3e591918b25e700229c9cdb3

saghul commented 4 months ago

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

chqrlie commented 4 months ago

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 commented 3 months ago

Gentle ping @chqrlie :-)

saghul commented 3 months ago

So, our implementation matches upstream now, 262 passes and I also tested with txiki.js: https://github.com/saghul/txiki.js/pull/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.

chqrlie commented 3 months ago

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 :)