Skip to content

Conversation

@MathiasVP
Copy link
Contributor

This snippet causes non-terminating modulus analysis when we remove IR phi input operands as SSA variables. I think it has something to do with the non-exact phi operand, but I've yet to verify this. For now it's good to simply have this testcase included

@MathiasVP MathiasVP requested a review from a team as a code owner November 7, 2023 11:56
@MathiasVP MathiasVP added the no-change-note-required This PR does not need a change note label Nov 7, 2023
@github-actions github-actions bot added C++ and removed no-change-note-required This PR does not need a change note labels Nov 7, 2023
@MathiasVP MathiasVP requested a review from geoffw0 November 7, 2023 11:57
while (x->n) {
x->n--;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

There are no expected results for the test. It might be nice to either add some and/or explain that we're mostly checking for an analysis non-termination issue here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed the issue here is specifically nontermination. I can add a comment if the function name doesn't make this clear 🙂

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, it's a little odd that we're supposed to read the function name as a comment on the implementation, though we're not exactly strict about such things. I'd prefer a comment. I don't have strong feelings.

@MathiasVP MathiasVP merged commit 2787f0a into github:main Nov 7, 2023
MathiasVP added a commit to MathiasVP/ql that referenced this pull request Nov 7, 2023
bdrodes pushed a commit to microsoft/codeql that referenced this pull request Nov 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants