Skip to content

Conversation

jketema
Copy link
Contributor

@jketema jketema commented Mar 7, 2023

Targeting the use-use dataflow feature branch, because I cannot be bothered to solve any merge conflicts when doing this on main.

@jketema jketema requested a review from a team as a code owner March 7, 2023 17:23
@github-actions github-actions bot added the C++ label Mar 7, 2023
@jketema jketema added the no-change-note-required This PR does not need a change note label Mar 7, 2023
Copy link
Contributor

@MathiasVP MathiasVP left a comment

Choose a reason for hiding this comment

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

LGTM! This was a lot of repetitive diff reading, so I'm relying quite a lot on tests to spot anything here 😵.

@jketema jketema removed the no-change-note-required This PR does not need a change note label Mar 7, 2023
Apparently some queries we skipped in the testing I did locally.
@jketema
Copy link
Contributor Author

jketema commented Mar 8, 2023

@MathiasVP Except for the caching issue DCA looks good. Should we hold this off, can we just merge?

@MathiasVP
Copy link
Contributor

@MathiasVP Except for the caching issue DCA looks good. Should we hold this off, can we just merge?

Hmm... I'd like to just merge it. It does put slightly more risk on the use-use flow branch for the upcoming LGTM run, but I don't think it's enough to be concerning.

@jketema
Copy link
Contributor Author

jketema commented Mar 8, 2023

In that case, let just go ahead if the changes from 0f8a12f and 5391b13 look good to you.

Copy link
Contributor

@MathiasVP MathiasVP left a comment

Choose a reason for hiding this comment

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

LGTM!

@MathiasVP MathiasVP merged commit e68bb53 into github:mathiasvp/replace-ast-with-ir-use-usedataflow Mar 8, 2023
@jketema jketema deleted the more-config branch March 8, 2023 13:26
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.

3 participants