Skip to content

feat(transformer/class-properties): support for transforming AssignmentTarget#7697

Merged
graphite-app[bot] merged 1 commit intomainfrom
12-06-feat_transformer_class-properties_support_for_transforming_assignmenttarget
Dec 6, 2024
Merged

feat(transformer/class-properties): support for transforming AssignmentTarget#7697
graphite-app[bot] merged 1 commit intomainfrom
12-06-feat_transformer_class-properties_support_for_transforming_assignmenttarget

Conversation

@Dunqing
Copy link
Member

@Dunqing Dunqing commented Dec 6, 2024

Instance prop:

  • [object.#prop] = arr -> [_toSetter(_classPrivateFieldSet2, [_prop, object])._] = arr
  • ({x: object.#prop} = obj) -> ({ x: _toSetter(_classPrivateFieldSet2, [_prop, object])._ } = obj)

Static prop:

  • [object.#prop] = arr -> [_assertClassBrand(Class, object, _prop)._] = arr
  • ({x: object.#prop} = obj) -> ({ x: _assertClassBrand(Class, object, _prop)._ } = obj)

@graphite-app
Copy link
Contributor

graphite-app bot commented Dec 6, 2024

Your org has enabled the Graphite merge queue for merging into main

Add the label “0-merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

@Dunqing Dunqing changed the title feat(transformer/class-properties): support for transforming AssignmentTarget feat(transformer/class-properties): support for transforming AssignmentTarget Dec 6, 2024
@github-actions github-actions bot added the A-transformer Area - Transformer / Transpiler label Dec 6, 2024
Copy link
Member Author

Dunqing commented Dec 6, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

@github-actions github-actions bot added the C-enhancement Category - New feature or request label Dec 6, 2024
@codspeed-hq
Copy link

codspeed-hq bot commented Dec 6, 2024

CodSpeed Performance Report

Merging #7697 will not alter performance

Comparing 12-06-feat_transformer_class-properties_support_for_transforming_assignmenttarget (86d4c90) with main (5f4f6d1)

Summary

✅ 29 untouched benchmarks

Copy link
Member

@overlookmotel overlookmotel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! So many test cases now green.

@overlookmotel overlookmotel added the 0-merge Merge with Graphite Merge Queue label Dec 6, 2024
Copy link
Member

overlookmotel commented Dec 6, 2024

Merge activity

  • Dec 6, 7:16 AM EST: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
  • Dec 6, 7:19 AM EST: A user added this pull request to the Graphite merge queue.
  • Dec 6, 7:23 AM EST: A user merged this pull request with the Graphite merge queue.

…entTarget` (#7697)

Instance prop:
 * `[object.#prop] = arr` -> `[_toSetter(_classPrivateFieldSet, [_prop, object])._] = arr`
 * `({x: object.#prop} = obj)` -> `({ x: _toSetter(_classPrivateFieldSet, [_prop, object])._ } = obj)`

 Static prop:
 * `[object.#prop] = arr` -> `[_assertClassBrand(Class, object, _prop)._] = arr`
 * `({x: object.#prop} = obj)` -> `({ x: _assertClassBrand(Class, object, _prop)._ } = obj)`
@overlookmotel overlookmotel force-pushed the 12-06-feat_transformer_class-properties_support_for_transforming_assignmenttarget branch from ac6801b to 86d4c90 Compare December 6, 2024 12:18
@graphite-app graphite-app bot merged commit 86d4c90 into main Dec 6, 2024
@graphite-app graphite-app bot deleted the 12-06-feat_transformer_class-properties_support_for_transforming_assignmenttarget branch December 6, 2024 12:23
Dunqing pushed a commit that referenced this pull request Dec 6, 2024
Follow-on after #7697. No need to cast twice. You can go from `MemberExpression` direct to `AssignmentTarget`.
Dunqing pushed a commit that referenced this pull request Dec 6, 2024
…ssignment_target` (#7701)

Follow-on after #7697.

`transform_assignment_target` is inlined into `enter_assignment_target` visitor, so we want it to be as small as possible. Move assigning to `target` into `transform_assignment_target_impl`, which is the cold path.

The principle is that in the transformer the most important thing for performance is to optimize for the path which is "nothing to do here, exit".

Here we are only transforming `object.#prop`, but very few `AssignmentTarget`s are `object.#prop`, so 99% of the time there is nothing to do. So we want to keep that "do nothing and exit" path as fast and small as possible.

In practice, the `*target =` assignment is only a single assembly operation, so this PR is a micro-optimization. But why not? Every little helps.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

0-merge Merge with Graphite Merge Queue A-transformer Area - Transformer / Transpiler C-enhancement Category - New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants