Skip to content

C++: Better caching for IR-based use-use dataflow#11645

Merged
MathiasVP merged 2 commits intogithub:mathiasvp/replace-ast-with-ir-use-usedataflowfrom
MathiasVP:more-caching
Dec 10, 2022
Merged

C++: Better caching for IR-based use-use dataflow#11645
MathiasVP merged 2 commits intogithub:mathiasvp/replace-ast-with-ir-use-usedataflowfrom
MathiasVP:more-caching

Conversation

@MathiasVP
Copy link
Copy Markdown
Contributor

@MathiasVP MathiasVP commented Dec 9, 2022

Turns out the simpleLocalFlowStep relation wasn't cached 😱. This PR fixes this.

@github-actions github-actions Bot added the C++ label Dec 9, 2022
@MathiasVP MathiasVP marked this pull request as ready for review December 10, 2022 14:51
@MathiasVP MathiasVP requested a review from a team as a code owner December 10, 2022 14:51
@MathiasVP MathiasVP added the no-change-note-required This PR does not need a change note label Dec 10, 2022
Copy link
Copy Markdown
Contributor

@jketema jketema left a comment

Choose a reason for hiding this comment

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

That diff was very hard to read, but LGTM.

@MathiasVP
Copy link
Copy Markdown
Contributor Author

That diff was very hard to read, but LGTM.

Yeah, sorry :( it's really just moving things into a cached module, but I do see that the diff is making this rather difficult to spot!

@MathiasVP MathiasVP merged commit 9e7b73a into github:mathiasvp/replace-ast-with-ir-use-usedataflow Dec 10, 2022
@jketema
Copy link
Copy Markdown
Contributor

jketema commented Dec 10, 2022

it's really just moving things into a cached module

Yup, that was also my conclusion.

@hvitved
Copy link
Copy Markdown
Contributor

hvitved commented Dec 10, 2022

Unless you invoke simpleLocalFlowStep elsewhere (which I wouldn't expect), caching it should not be needed, since the shared data flow library makes sure to cache it.

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.

3 participants