Skip to content
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

C++: Change countNumberOfBranchesUsingParameter to match qldoc closer. #18908

Merged

Conversation

aschackmull
Copy link
Contributor

@aschackmull aschackmull commented Mar 3, 2025

This changes one ad-hoc fieldFlowBranchLimit tweak to another. I tried to do some comparisons on the computed counts, and there are plenty of discrepancies, but that's possibly completely fine - let's see what dca says.

The primary motivation for this change is to get rid of the dependence on SSA Phi Read nodes, which I plan to hide within the SSA library.

@github-actions github-actions bot added the C++ label Mar 3, 2025
@aschackmull
Copy link
Contributor Author

Dca looks fine, so opening this for review. @MathiasVP looks like there was an issue with QA - do you want to retry that or is the dca run sufficient?

@aschackmull aschackmull added the no-change-note-required This PR does not need a change note label Mar 4, 2025
@aschackmull aschackmull marked this pull request as ready for review March 4, 2025 07:38
@Copilot Copilot bot review requested due to automatic review settings March 4, 2025 07:38
@aschackmull aschackmull requested a review from a team as a code owner March 4, 2025 07:38

Choose a reason for hiding this comment

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

Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.

Tip: If you use Visual Studio Code, you can request a review from Copilot before you push from the "Source Control" tab. Learn more

@MathiasVP
Copy link
Contributor

Ì think we should redo QA. I'll restart it now

@aschackmull
Copy link
Contributor Author

QA looks sufficiently uneventful, I think?

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.

Indeed 😄 LGTM!

@MathiasVP MathiasVP merged commit 38bf9c6 into github:main Mar 5, 2025
15 checks passed
@aschackmull aschackmull deleted the cpp/branchlimit-adjustment-refactor branch March 5, 2025 11:56
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