Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Single (+patchable) LiteralNode #3536

Merged
merged 9 commits into from
Jun 21, 2022
Merged

Conversation

JaroslavTulach
Copy link
Member

[ci no changelog needed]

Pull Request Description

This PR merges existing variants of LiteralNode (Integer, BigInteger, Decimal, Text) into a single LiteralNode. It adds PatchableLiteralNode variant (with non final value field) and uses Node.replace to modify the AST to be patchable. With such change one can remove the UnwindHelper workaround as IdExecutionInstrument now sees patched return values without any tricks.

Checklist

Please include the following checklist in your PR:

  • [ x ] All code conforms to the
    Scala,
    Java,
  • All code has been tested:
    • [ x ] Unit tests have been written where possible.

@JaroslavTulach JaroslavTulach force-pushed the wip/jtulach/OneLiteralNode_182485854 branch from 8a032aa to 029b027 Compare June 21, 2022 08:57
@JaroslavTulach
Copy link
Member Author

Results of running benchmarks seem to be fine:

  • RecursionBenchmarks are OK
  • NamedDefaultedArgumentBenchmarks are OK
  • CallableBenchmarks are OK
  • RuntimeCacheBenchmarks is OK

The AtomBenchmarks are currently broken, but I don't expect any regression either. Time to request integration.

@JaroslavTulach JaroslavTulach added the CI: Ready to merge This PR is eligible for automatic merge label Jun 21, 2022
@mergify mergify bot merged commit 8f63679 into develop Jun 21, 2022
@mergify mergify bot deleted the wip/jtulach/OneLiteralNode_182485854 branch June 21, 2022 09:42
Copy link
Collaborator

@hubertp hubertp left a comment

Choose a reason for hiding this comment

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

nits

Copy link
Contributor

@kustosz kustosz left a comment

Choose a reason for hiding this comment

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

Looks good!

kazcw pushed a commit that referenced this pull request Jun 29, 2022
This PR merges existing variants of `LiteralNode` (`Integer`, `BigInteger`, `Decimal`, `Text`) into a single `LiteralNode`. It adds `PatchableLiteralNode` variant (with non `final` `value` field) and uses `Node.replace` to modify the AST to be patchable. With such change one can remove the `UnwindHelper` workaround as `IdExecutionInstrument` now sees _patched_ return values without any tricks.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants