peggyjs / peggy

Peggy: Parser generator for JavaScript
https://peggyjs.org/
MIT License
906 stars 64 forks source link

Fixes to prepare generate-bytecode.js for ts-check #452

Closed markw65 closed 8 months ago

markw65 commented 8 months ago

I started to add jsdoc type annotations to generate-bytecode.js, and found a number of issues that typescript doesn't like. It seemed to make sense to fix those separately. But I can add the actual ts-check pull request (not yet submitted) to this one if you'd prefer.

  1. Using thing | 0 to produce an integer when thing could be undefined

    • this can be useful in some cases - eg if thing might be boolean, or a non-integer, or an integer outside the range of a 32 bit number. But in every case, it's a match field which is always one of -1, 0, 1 or undefined. So thing || 0 does the same thing, and typescript is happy with it.
  2. Putting null into arrays that are supposed to contain numbers

    • In every case it was to avoid adding something to one of the literal arrays, when the match field says that it won't be used. So using -1 is just as good.
    1. Using Object.entries with lib set to ES2015
      • If you think ES2017 is universal enough, we could just bump the lib version. With this diff I've gone with switching to Object.keys.
hildjj commented 8 months ago
  1. Last time that we discussed |0, I recall @Mingun having opinions. Typescript's reaction might change their mind.
  2. nod
  3. I think switching to Object.keys is probably better for now, even though it's been 'broken' for long enough that we can probably just switch lib version. I'd like to have a larger discussion about version support after we refactor code generation.
Mingun commented 8 months ago

|0 is used only as idiomatic way to convert everything into an integer in JS. In theory, that also should help JIT that will see that the property variable has only integer shape. If something changed since those times, you can refactor code to reflect that.

hildjj commented 8 months ago

While I agree that |0 is probably still better, a) Typescript doesn't understand it and b) I don't think this is going to make enough performance difference to matter, so let's move forward.

@markw65 do you want me to merge this now, or would you like to add another commit that turns on ts-check first? (regardless, will need a changelog)

markw65 commented 8 months ago

Let's just go with this. I'll send a ts-check pull request shortly.

Btw, @Mingun is right that its faster:

node -p 'const start=Date.now(); let x = 0; for (let i = 0; i < 1000000000; i += (x || 0) + 1) {}; Date.now() - start'
1402
node -p 'const start=Date.now(); let x = 0; for (let i = 0; i < 1000000000; i += (x | 0) + 1) {}; Date.now() - start'
698

Similar results if you start with let x = undefined.

But at an extra 700ms for a billion iterations, I don't think it would be possible to measure the impact on peggy...