Skip to content

C++: Fix more asExpr duplication #15458

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 2 commits into from
Jan 30, 2024

Conversation

MathiasVP
Copy link
Contributor

@MathiasVP MathiasVP commented Jan 29, 2024

When we check for whether an instruction represents the "fully converted expression" we ask for the expression associated with the instruction, and require that no "later" instruction associated with the same expression exists. This last part is done via the integer n in the charpred that looks like:

exists(Expr e, int n |
  exprNodeShouldBeInstruction(this, e, n) and
  not exprNodeShouldBe(e, n + 1)
)

However, it may actually be the case that one instruction is associated with one conversion of an instruction, but a later instruction is associated with another conversion of the same expression. To account for this, instead of requiring that there doesn't exist another instruction with a larger n and the same instruction, we ask whether there exists an instruction with a larger n and a conversion of the same unconverted instruction. That is, we now do:

exists(Expr e, int n |
  exprNodeShouldBeInstruction(this, e, n) and
  not exists(Expr conv |
    exprNodeShouldBe(conv, n + 1) and
    conv.getUnconverted() = e.getUnconverted()
  )
)

and this fixes the remaining dataflow node duplication in our asExpr tests 🎉

@github-actions github-actions bot added the C++ label Jan 29, 2024
@MathiasVP MathiasVP marked this pull request as ready for review January 29, 2024 16:36
@MathiasVP MathiasVP requested a review from a team as a code owner January 29, 2024 16:36
@MathiasVP MathiasVP added the no-change-note-required This PR does not need a change note label Jan 29, 2024
@MathiasVP MathiasVP requested a review from geoffw0 January 29, 2024 17:38
@MathiasVP MathiasVP merged commit 56e44f9 into github:main Jan 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C++ 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.

2 participants