Skip to content

C++: Handle nonreturning functions in IR generation #12982

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
Merged
merged 8 commits into from
May 3, 2023
Merged

C++: Handle nonreturning functions in IR generation #12982

merged 8 commits into from
May 3, 2023

Conversation

rdmarsh2
Copy link
Contributor

@rdmarsh2 rdmarsh2 commented May 2, 2023

No description provided.

@rdmarsh2 rdmarsh2 requested review from a team as code owners May 2, 2023 03:13
MathiasVP
MathiasVP reviewed May 3, 2023
View reviewed changes
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.

The code LGTM (assuming the DCA run I started looks good). I have one small comment on the change note. I'd like to hear @jketema's opinion on the PR as well given that he reviewed most of the original PR

jketema
jketema reviewed May 3, 2023
View reviewed changes
Copy link
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.

Code looks good to me. DCA analysis times look good. The alert changes for git make sense to me. I've a hard time grasping the OpenTTD alert change from just the DCA output.

@jketema
Copy link
Contributor

jketema commented May 3, 2023

I've a hard time grasping the OpenTTD alert change from just the DCA output.

Looking at this locally now so I can see the dataflow path.

@jketema
Copy link
Contributor

jketema commented May 3, 2023

Looking at this locally now so I can see the dataflow path.

This also makes sense to me. It's not completely clear to me why this path was missing before, but I don't want to dwell on that for too long unless you tell me otherwise. I do see some functions calls in the function with the sink that would now cause an unreachable instruction to be generated.

@MathiasVP
Copy link
Contributor

Yeah, I don't think we need to dwell too much into it either.

MathiasVP
MathiasVP approved these changes May 3, 2023
View reviewed changes
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!

@jketema jketema merged commit 0b2f560 into github:main May 3, 2023
@jketema jketema mentioned this pull request May 4, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jketema jketema jketema left review comments

@MathiasVP MathiasVP MathiasVP approved these changes

Assignees
No one assigned
Labels
C# C++ documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rdmarsh2 @jketema @MathiasVP