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.

Sign up for GitHub

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

Jump to bottom

Handle while true (Fixes #3249) #3389

Merged
merged 4 commits into from
Jun 30, 2020
Merged

Handle while true (Fixes #3249) #3389

merged 4 commits into from
Jun 30, 2020

Conversation

d367wang
Copy link
Contributor

@d367wang d367wang commented Jun 17, 2020 •
edited by smillst
Loading

This PR fixes both issue #3249 and issue #1727, by adapting the CFG in the following way:

If the loop condition is checked to be a constant true (according to the same logic in javac flow analysis), then the loop condition does not have an else-branch, i.e. the block holding the loop condition is directly followed by the one holding the loop entry, instead of followed by a conditional block.

Fixes #1727 and Fixes #3249`

@wmdietl wmdietl mentioned this pull request Jun 18, 2020
Closed
@wmdietl wmdietl changed the title fix issue#3249 Handle while true (Fixes #3249) Jun 18, 2020
wmdietl
wmdietl approved these changes Jun 18, 2020
View reviewed changes
Copy link
Member

@wmdietl wmdietl left a comment

Choose a reason for hiding this comment

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

Thanks for this fix!

@wmdietl wmdietl linked an issue Jun 18, 2020 that may be closed by this pull request
CFG for while (true) #3249
Closed
@smillst smillst self-assigned this Jun 22, 2020
smillst
smillst requested changes Jun 24, 2020
View reviewed changes
Copy link
Member

@smillst smillst left a comment

Choose a reason for hiding this comment

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

These changes look good. I have two minor comments.

FYI, when a pull request fixes an issue, then the pull request description should say fixes #XXXX or fixes #XXXX and fixes #XXXX. That way those issues will be closed when the pull request is merge. You could use one of the other ways. I've updated this pull request description.

dataflow/src/main/java/org/checkerframework/dataflow/cfg/CFGBuilder.java Outdated
@@ -4913,15 +4915,30 @@ public Node visitWhileLoop(WhileLoopTree tree, Void p) {
// Condition
addLabelForNextNode(conditionStart);
assert tree.getCondition() != null;
// Determine whether loop condition is a constant true, according to the compiler logic.
Copy link
Member

Choose a reason for hiding this comment

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

true is a literal not a constant. So this should say Determine whether loop condition is the boolean literal "true",. You've used constant instead of literal in other places in this pull request. Please change all uses.

Copy link
Member

Choose a reason for hiding this comment

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

More generally, a literal appears in source code, and a constant is an expression that can be evaluated at compile time. So, true is both a literal and a constant. 4 > 3 is a constant.

Using these terms consistently will help to make the code easier to read. Thanks!

(I haven't read the code to determine whether it is checking for the literal true or the constant true.)

Copy link
Member

Choose a reason for hiding this comment

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

@smillst This PR correctly uses constant. The compiler handles "!false" and "6 > 4" as being the compile-time constant true.

@d367wang Please add the following test case:

    Issue3249() {
        double local;
        while (6 > 4) {
            local = 1;
            break;
        }
        field = local;
    }

dataflow/src/main/java/org/checkerframework/dataflow/cfg/CFGBuilder.java Outdated
@@ -4985,6 +5002,35 @@ public Node visitOther(Tree tree, Void p) {
}
}

/**
* Determine whether loop condition {@link JCTree} is a constant true, according to the compiler
Copy link
Member

Choose a reason for hiding this comment

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

This method works for any tree, even if that tree is not a loop condition. We might find uses for this method outside of this class, would you please move it to TreeUtils and rename it to isLiteralTrue.

Copy link
Member

Choose a reason for hiding this comment

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

Moving to TreeUtils sounds good. As name I would use isExpressionConstantTrue or isExprConstTrue, or the current isCondConstTrue.

Copy link
Member

Choose a reason for hiding this comment

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

The Javadoc should say is a compile-time constant true or as the JLS uses a condition expression has the constant value true. (I clearly misunderstood the term constant true, other might, too.)

@smillst smillst assigned d367wang and unassigned smillst Jun 24, 2020
@wmdietl
Copy link
Member

wmdietl commented Jun 24, 2020

FYI, when a pull request fixes an issue, then the pull request description should say fixes #XXXX or fixes #XXXX and fixes #XXXX. That way those issues will be closed when the pull request is merge. You could use one of the other ways. I've updated this pull request description.

What do you mean here? I renamed this issue 7 days ago an don't see a further edit.

@smillst
Copy link
Member

smillst commented Jun 24, 2020

@wmdietl, you changed the title of the issue. I updated the pull request description which is the first comment. If you add fixes #XXXX in the title, it doesn't actually link the issue, which is why you had to manually link it later. Though if you just have fixes #XXXX in the title, it will still close the issue when it is merged because it is in the git commit message.

@wmdietl
Copy link
Member

wmdietl commented Jun 24, 2020

@wmdietl, you changed the title of the issue.

Here do you mean "pull request" instead of "issue"?

You're pointing out that even though the first sentence of the PR description is This PR fixes both issue #3249 and issue #1727 you added the last sentence Fixes #1727 and Fixes #3249 to actually also convince GitHub to link the issues.

@smillst smillst assigned smillst and unassigned d367wang Jun 29, 2020
@smillst smillst added this to the Critical milestone Jun 29, 2020
smillst
smillst approved these changes Jun 30, 2020
View reviewed changes
Copy link
Member

@smillst smillst left a comment

Choose a reason for hiding this comment

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

Thanks for the improvements!

@smillst smillst merged commit c5f2eb8 into typetools:master Jun 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mernst mernst mernst left review comments

@wmdietl wmdietl wmdietl approved these changes

@smillst smillst smillst approved these changes

Assignees

@smillst smillst

Labels
None yet
Projects
None yet
Milestone
Critical
Development

Successfully merging this pull request may close these issues.

CFG for while (true) Initialization problem in while true loop
4 participants
@d367wang @wmdietl @smillst @mernst