Skip to content

SSA: Distinguish between has and controls branch edge. #19567

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

Merged
merged 3 commits into from
May 23, 2025

Conversation

aschackmull
Copy link
Contributor

For languages without a guards implication logic, hasBranchEdge and controlsBranchEdge is the same thing, but libraries like SSA should distinguish between them. This might theoretically improve barrier guards a bit for Java, but more importantly this prepares us a bit for the introduction of a shared Guards library.

@Copilot Copilot AI review requested due to automatic review settings May 23, 2025 08:00
@aschackmull aschackmull requested review from a team as code owners May 23, 2025 08:00
Copilot

This comment was marked as off-topic.

/**
* Holds if this guard evaluating to `branch` controls the control-flow
* branch edge from `bb1` to `bb2`. That is, following the edge from
* `bb1` to `bb2` implies that this guard evaluated to `branch`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps add an example to illustrate how this can differ from hasBranchEdge.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the general distinction between direct and indirect control. E.g. if (g) .. vs. b = g; .. if (b) ... I can of course elaborate the qldoc here, but I'm thinking perhaps just leaving it as-is, and then focusing on a more comprehensive elaboration in the upcoming shared Guards library - eventually these two predicates will simply be taken directly from that shared library.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that is what I was thinking; simply add that example to the QL doc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@aschackmull aschackmull added no-change-note-required This PR does not need a change note and removed no-change-note-required This PR does not need a change note labels May 23, 2025
@aschackmull
Copy link
Contributor Author

Dca looks completely fine. This is technically a breaking change for any users of the SSA data flow integration module, so I've added a change note to the SSA qlpack.

@aschackmull aschackmull merged commit 1b2d23b into github:main May 23, 2025
59 checks passed
@aschackmull aschackmull deleted the ssa/branchedge branch May 23, 2025 11:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants