Closed Bug 1454744 Opened Last year Closed Last year

[BinAST] Create proper bindings for positional formal parameters

Categories

(Core :: JavaScript Engine, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: efaust, Assigned: efaust)

Details

Attachments

(2 files)

No description provided.
This is modeled after (read: cargo culted) from the fields stuff. I tried commoning up the logic with some callbacks, but the helper got more complicated than the code duplication, so this is where I landed.

I admit that in theory this isn't a great place to put parsing code, since it's just the structural induction step, but it's pretty convenient here.
Attachment #8968636 - Flags: review?(dteller)
Comment on attachment 8968636 [details] [diff] [review]
Part 1: Augment BinSource parser generator to allow code in sum arms

Review of attachment 8968636 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm

::: js/src/frontend/binsource/src/main.rs
@@ +73,4 @@
>      /// Custom per-field treatment. Used only for interfaces.
>      by_field: HashMap<FieldName, FieldRules>,
>  
> +    by_sum: HashMap<NodeName, SumRules>,

Nit: Could you document this?

@@ +775,4 @@
>              (rules_for_this_node.build_result.is_some(), "build:"),
>              (rules_for_this_node.append.is_some(), "append:"),
>              (rules_for_this_node.by_field.len() > 0, "fields:"),
> +            (rules_for_this_node.by_sum.len() > 0, "sum-arms:"),

Could you add this same verification to `fn generate_implement_interface`? Not urgent, that can wait for a followup bug.
Attachment #8968636 - Flags: review?(dteller) → review+
Comment on attachment 8968637 [details] [diff] [review]
Part 2: add bindings for formal positional parameters

Review of attachment 8968637 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm.

However, sums are footgunish as the deanonymizer needs to transform `Foo = A|Bar, Bar = C|D` into `Foo = A|C|D, Bar = C|D`, so D ends up appearing in several places. So, can you double-check in the generated that you have applied it in all the places you meant to apply it?

I have code in master to mitigate this problem, but I have not attempted to use it for binsource/src/main.rs. This might be needed for this use case.
Attachment #8968637 - Flags: review?(dteller) → review+
Priority: -- → P2
Pushed by efaustbmo@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/42c4d4acfba5
Part 1: Allow BinSource generator to place code after parsing interface sum branches. (r=Yoric)
https://hg.mozilla.org/integration/mozilla-inbound/rev/02e7d188fd56
Part 2: Add bindings for formal parameter names. (r=Yoric)
https://hg.mozilla.org/mozilla-central/rev/42c4d4acfba5
https://hg.mozilla.org/mozilla-central/rev/02e7d188fd56
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.