Skip to content

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 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

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