Skip to content

Conversation

@aschackmull
Copy link
Contributor

Commit-by-commit review encouraged. I've attempted to make every commit self-contained.

This PR includes a sequence of small changes, including:

  • A bugfix for guards for SSA input nodes. The version on main accidentally had a cast from DefinitionExt to Definition that ruled out some guarded nodes.
  • Replacement of the use-use calculation to use the revised implementation.
  • Switch the JavaScript ad-hoc barrier-guard for falsy checks to use the out-of-the-box implementation. (This adds a few additional guarded nodes in the case I tested - no tuples were lost).
  • Deprecate the public SsaDefinitionExtNode and SsaInputNode and replace them with a more general notion of synthesized reads.

@aschackmull aschackmull added the no-change-note-required This PR does not need a change note label Feb 25, 2025
Copilot AI review requested due to automatic review settings February 25, 2025 09:14
@aschackmull aschackmull requested review from a team as code owners February 25, 2025 09:14
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.

Tip: Copilot only keeps its highest confidence comments to reduce noise and keep you focused. Learn more

@aschackmull aschackmull requested a review from hvitved February 25, 2025 09:14
@github-actions github-actions bot added C# JS Java Ruby Rust Pull requests that update Rust code labels Feb 25, 2025
@aschackmull aschackmull force-pushed the ssa/refactor-df-integr branch from 5e2c8f2 to 449150e Compare February 25, 2025 09:42
Copy link
Contributor

@asgerf asgerf left a comment

Choose a reason for hiding this comment

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

JS changes LGTM

Copy link
Contributor

@hvitved hvitved left a comment

Choose a reason for hiding this comment

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

LGTM, only one question.

*/
class SsaInputNode extends SsaNode {
override SsaImpl::DataFlowIntegration::SsaInputNode node;
SsaImpl::Definition getDefinition() { result = node.getDefinition() }
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you change the return type to Ssa::Definition and avoid the infix cast below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, done.

@aschackmull aschackmull force-pushed the ssa/refactor-df-integr branch from e2c183d to 28e9644 Compare February 25, 2025 13:13
@aschackmull aschackmull merged commit 994a8ee into github:main Feb 25, 2025
55 checks passed
@aschackmull aschackmull deleted the ssa/refactor-df-integr branch February 25, 2025 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C# Java JS no-change-note-required This PR does not need a change note Ruby Rust Pull requests that update Rust code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants