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

Java: Make separate classes for different control flow node kinds #17996

Merged
merged 3 commits into from
Dec 12, 2024

Conversation

owen-mc
Copy link
Contributor

@owen-mc owen-mc commented Nov 15, 2024

This PR is built on top of #17970. That should be merged first. Only review after the commit "Undo accidental bugfix".

This PR is purely a refactoring to make the code easier to read and to make it easier to add new kinds of control flow nodes. As a guide I used the equivalent class in the go library.

@owen-mc owen-mc added the no-change-note-required This PR does not need a change note label Nov 15, 2024
@owen-mc owen-mc requested a review from a team as a code owner November 15, 2024 11:14
private ControlFlowNode mainBranchSucc(ControlFlowNode n, boolean b) {
result = succ(n, BooleanCompletion(_, b))
}
private Node mainBranchSucc(Node n, boolean b) { result = succ(n, BooleanCompletion(_, b)) }

Check warning

Code scanning / CodeQL

Missing QLDoc for parameter Warning

The QLDoc has no documentation for b, or n, but the QLDoc mentions booleanCompletion
@@ -1487,8 +1602,8 @@
* In the latter case, when `n` occurs as the last node in a finally block, there might be
* multiple different such successors.
*/
private ControlFlowNode otherBranchSucc(ControlFlowNode n, boolean b) {
exists(ControlFlowNode main | main = mainBranchSucc(n, b.booleanNot()) |
private Node otherBranchSucc(Node n, boolean b) {

Check warning

Code scanning / CodeQL

Missing QLDoc for parameter Warning

The QLDoc has no documentation for b, but the QLDoc mentions booleanCompletion, and normalCompletion
@owen-mc owen-mc force-pushed the java/lightweight-IR-layer-classes branch from 86804e2 to 2ca2a4b Compare November 15, 2024 11:22
@owen-mc
Copy link
Contributor Author

owen-mc commented Nov 15, 2024

Two language tests are failing. One can only be fixed by an internal PR, which must be merged at the same time as #17970. The other will be fixed by #17988. Integration tests are failing for unrelated reasons.

This puts all the logic of a particular control flow node kind into one
place and makes it easier to add new kinds.
Put all the ones which might need to be overrridden by subclasses
together for ease of reading.
@owen-mc owen-mc force-pushed the java/lightweight-IR-layer-classes branch from 359ee98 to aaa4361 Compare December 11, 2024 10:35
egregius313
egregius313 previously approved these changes Dec 11, 2024
Copy link
Contributor

@egregius313 egregius313 left a comment

Choose a reason for hiding this comment

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

LGTM

@owen-mc owen-mc requested a review from aschackmull December 12, 2024 10:28
It doesn't really make sense to define them in terms of dispatch.
@owen-mc owen-mc merged commit 8703e21 into github:main Dec 12, 2024
14 checks passed
@owen-mc owen-mc deleted the java/lightweight-IR-layer-classes branch December 12, 2024 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Java no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants