1456609 - [BinAST] Expose the APIs necessary for skipping skippable nodes

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P2)

Comment 1

[David Teller [:Yoric] - still alive but not very active]

Attachment: Bug 1456609 - Expose a method readSkippableSubTree for BinTokenReader{Tester, Multipart}

Review commit: https://reviewboard.mozilla.org/r/239612/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/239612/

Comment 2

[Tooru Fujisawa [:arai]]

Comment on attachment 8970834 [details]
Bug 1456609 - Expose a method readSkippableSubTree for BinTokenReader{Tester, Multipart}

https://reviewboard.mozilla.org/r/239612/#review245560

I have some questions.

::: js/src/frontend/BinSource-auto.cpp:6174
(Diff revision 1)
> +#endif // defined(DEBUG)
> +
> +
> +
> +
> +    BINJS_MOZ_TRY_DECL(offset, tokenizer_->readSkippableSubTree());

why is this assigning the tree to a variable named `offset`?
(instead of `skipped` or something?)
(maybe related to the change in main.rs)

also, where does the `BinField::Offset` come from?
there's only `skipped` field in SkippableSOMETHING interfaces.

::: js/src/frontend/binsource/src/main.rs:1002
(Diff revision 1)
> -                        (Some(format!("uint32_t {var_name};", var_name = var_name)),
> -                        Some(format!("MOZ_TRY_VAR({var_name}, tokenizer_->readOffset());", var_name = var_name)))
> +                        (Some(format!("BinTokenReaderBase::SkippableSubTree {var_name};", var_name = var_name)),
> +                        Some(format!("MOZ_TRY_VAR({var_name}, tokenizer_->readSkippableSubTree());", var_name = var_name)))
>                      } else {
>                          (None,
> -                        Some(format!("BINJS_MOZ_TRY_DECL({var_name}, tokenizer_->readOffset());", var_name = var_name)))
> +                        Some(format!("BINJS_MOZ_TRY_DECL({var_name}, tokenizer_->readSkippableSubTree());", var_name = var_name)))

this sounds strange.
this block is handling `offset`, right?
why are you modifying this block to handle `SkippableSubTree` ?
and how this generates the file in the patch?

Comment 3

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8970834 [details]
Bug 1456609 - Expose a method readSkippableSubTree for BinTokenReader{Tester, Multipart}

https://reviewboard.mozilla.org/r/239612/#review245560

> why is this assigning the tree to a variable named `offset`?
> (instead of `skipped` or something?)
> (maybe related to the change in main.rs)
> 
> also, where does the `BinField::Offset` come from?
> there's only `skipped` field in SkippableSOMETHING interfaces.

The `_offset` field (which is normalized to `offset` in C++) and the `Offset` data structure are magically added during grammar preprocessing to every node labelled `[Skippable]`. This is done by the crate `binjs_meta`, which is vendored. I agree that name `_offset` is ill-chosen, so there is a patch to `binjs_meta` that renames it `_skip`.

As this is not observable, I just didn't want to vendor the new version just yet, as I already expect efaust's merging to be pretty complicated, and I didn't want to make it even worse.

Comment 4

[Tooru Fujisawa [:arai]]

Comment on attachment 8970834 [details]
Bug 1456609 - Expose a method readSkippableSubTree for BinTokenReader{Tester, Multipart}

https://reviewboard.mozilla.org/r/239612/#review245624

Thank you for explanation :)

Comment 5

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8970834 [details]
Bug 1456609 - Expose a method readSkippableSubTree for BinTokenReader{Tester, Multipart}

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/239612/diff/1-2/

Comment 6

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8970834 [details]
Bug 1456609 - Expose a method readSkippableSubTree for BinTokenReader{Tester, Multipart}

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/239612/diff/2-3/

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b917e7e80a51
Expose a method readSkippableSubTree for BinTokenReader{Tester, Multipart} r=arai

Comment 8

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/b917e7e80a51