Skip to content

Don't call bit_width if value is nullptr in _Fmt_write#2097

Merged
StephanTLavavej merged 11 commits intomicrosoft:mainfrom
SiliconA-Z:Width
Aug 27, 2021
Merged

Don't call bit_width if value is nullptr in _Fmt_write#2097
StephanTLavavej merged 11 commits intomicrosoft:mainfrom
SiliconA-Z:Width

Conversation

@SiliconA-Z
Copy link
Contributor

No description provided.

@SiliconA-Z SiliconA-Z requested a review from a team as a code owner August 5, 2021 22:19
@CaseyCarter CaseyCarter added the enhancement Something can be improved label Aug 6, 2021
@SiliconA-Z
Copy link
Contributor Author

Done!

@SiliconA-Z
Copy link
Contributor Author

Also this is technically a performance patch since we do the check anyway, but now moving it first before we call the function in the event value is nullptr

@StephanTLavavej StephanTLavavej added performance Must go faster and removed enhancement Something can be improved labels Aug 8, 2021
@SiliconA-Z
Copy link
Contributor Author

@miscco done!

Copy link
Contributor

@barcharcraz barcharcraz left a comment

Choose a reason for hiding this comment

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

This looks correct, but I'm not 100% convinced it's a performance improvement except in debug mode. But debug perf is important too, so I'm fine with it.

@barcharcraz barcharcraz removed their assignment Aug 16, 2021
@CaseyCarter CaseyCarter self-assigned this Aug 16, 2021
@SiliconA-Z
Copy link
Contributor Author

Issues addressed!

@SiliconA-Z SiliconA-Z requested a review from CaseyCarter August 17, 2021 18:39
@SiliconA-Z
Copy link
Contributor Author

So when can we merge, if I may ask?

@celonymire
Copy link
Contributor

So when can we merge, if I may ask?

They usually merge multiple PRs at the same time, so it may take a while.

@StephanTLavavej StephanTLavavej self-assigned this Aug 26, 2021
@StephanTLavavej
Copy link
Member

I'm mirroring this to an MSVC-internal PR. Please notify me if any further changes are pushed.

(As @sam20908 mentioned, the mirroring process is semi-manual, and requires enough effort that we batch up PRs in order to save dev time. Your PR will be part of the next batch of 17 PRs. There's no set schedule for mirroring, although it tends to be completed at the end of each week.)

@StephanTLavavej StephanTLavavej merged commit c1920e3 into microsoft:main Aug 27, 2021
@StephanTLavavej
Copy link
Member

Thanks for improving this codepath! 🚀 🎉 😸

@SiliconA-Z SiliconA-Z deleted the Width branch August 30, 2021 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

performance Must go faster

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants