Skip to content

Conversation

@Dunqing
Copy link
Member

@Dunqing Dunqing commented Oct 23, 2024

These two APIs are used to create a symbol with the provided symbol name, The difference from generate_uid is that we don't need to create a unique name for the symbol.

If you have a better method name for this, Feel free to edit this PR directly

@graphite-app
Copy link
Contributor

graphite-app bot commented Oct 23, 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.

Copy link
Member Author

Dunqing commented Oct 23, 2024

@Dunqing Dunqing changed the title feat(traverse): add generate_binding and generate_binding_current_scope APIs in context feat(traverse): add generate_binding and generate_binding_current_scope APIs in context Oct 23, 2024
@github-actions github-actions bot added the C-enhancement Category - New feature or request label Oct 23, 2024
@codspeed-hq
Copy link

codspeed-hq bot commented Oct 23, 2024

CodSpeed Performance Report

Merging #6805 will not alter performance

Comparing 10-23-feat_traverse_add_generate_binding_and_generate_binding_current_scope_apis_in_context (c96e739) with main (22355f7)

Summary

✅ 30 untouched benchmarks

@Dunqing Dunqing requested a review from overlookmotel October 23, 2024 08:11
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.

I think it'd be preferable if generate_binding took an Atom instead of a CompactStr. If it's an existing var name (which it must be, otherwise we'd be using generate_uid), an Atom will already exist in the arena for this symbol name, so we'd be better off reusing it. I'll submit that change in a follow-on PR.

This business with juggling CompactStrs and Atoms is a real pain!

@overlookmotel overlookmotel added the 0-merge Merge with Graphite Merge Queue label Oct 23, 2024 — with Graphite App
@graphite-app
Copy link
Contributor

graphite-app bot commented Oct 23, 2024

Merge activity

…scope` APIs in context (#6805)

These two APIs are used to create a symbol with the provided symbol name, The difference from `generate_uid` is that we don't need to create a unique name for the symbol.

If you have a better method name for this, Feel free to edit this PR directly
@overlookmotel overlookmotel force-pushed the 10-23-feat_traverse_add_generate_binding_and_generate_binding_current_scope_apis_in_context branch from aad68b0 to c96e739 Compare October 23, 2024 15:50
@graphite-app graphite-app bot merged commit c96e739 into main Oct 23, 2024
@graphite-app graphite-app bot deleted the 10-23-feat_traverse_add_generate_binding_and_generate_binding_current_scope_apis_in_context branch October 23, 2024 15:55
Dunqing pushed a commit that referenced this pull request Oct 24, 2024
…6830)

Follow-up after #6805.

`TraverseCtx::generate_binding` take an `Atom` instead of a `CompactStr`. If it's an existing var name (which it must be, otherwise we'd be using `TraverseCtx::generate_uid`), an `Atom` will already exist in the arena for this symbol name, so we'd be better off reusing it, rather than writing the same `Atom` into the arena again.
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 C-enhancement Category - New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants