Skip to content

C++: Make getUnderlyingType nomagic #8507

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
Mar 22, 2022
Merged

C++: Make getUnderlyingType nomagic #8507

merged 2 commits into from
Mar 22, 2022

Conversation

geoffw0
Copy link
Contributor

@geoffw0 geoffw0 commented Mar 21, 2022

Motivated by some observations of cpp/system-data-exposure (by DCA), but the effects of this change will not be limited to this project, and may or may not be positive overall. So I'll do a DCA run on this PR, then we can discuss...

@geoffw0 geoffw0 added the C++ label Mar 21, 2022
@geoffw0 geoffw0 requested a review from a team as a code owner March 21, 2022 11:20
@geoffw0 geoffw0 added the no-change-note-required This PR does not need a change note label Mar 21, 2022
@geoffw0
Copy link
Contributor Author

geoffw0 commented Mar 22, 2022

DCA:

  • none of the flagged items (build times, one alert change) are likely explained by this change.
    • but sooner or later we're going to get bitten by the unreliability of DCA and how easily we [have to] dismiss results.
  • overall analysis time was quicker but by an insignificant amount.

So DCA suggests this change is safe. I believe it helps in certain cases. And @MathiasVP has said he believes it is a good idea.

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 5cdf0b5 into github:main Mar 22, 2022
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