Closed Bug 1454744 Opened 4 years ago Closed 4 years ago

[BinAST] Create proper bindings for positional formal parameters


(Core :: JavaScript Engine, enhancement, P2)




Tracking Status
firefox61 --- fixed


(Reporter: efaust, Assigned: efaust)



(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]:


::: js/src/frontend/binsource/src/
@@ +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]:


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/ This might be needed for this use case.
Attachment #8968637 - Flags: review?(dteller) → review+
Priority: -- → P2
Pushed by
Part 1: Allow BinSource generator to place code after parsing interface sum branches. (r=Yoric)
Part 2: Add bindings for formal parameter names. (r=Yoric)
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.