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

Remove rarely true check in char_traits::move #2083

Merged
merged 1 commit into from
Aug 6, 2021

Conversation

AreaZR
Copy link
Contributor

@AreaZR AreaZR commented Jul 29, 2021

While it is faster to just leave when the source and destination are the same, most programmers would not make that mistake, and if they did, it would be a rarity and a reflection of something going wrong in their code in the first place.

There is no reason to do this check if speed and code size are of utmost importance.

@AreaZR AreaZR requested a review from a team as a code owner July 29, 2021 16:46
@AreaZR AreaZR changed the title Remove rarely true check Remove rarely true check in xstring move Jul 29, 2021
@AreaZR AreaZR changed the title Remove rarely true check in xstring move Remove rarely true check in char_traits::move Jul 29, 2021
@AdamBucior
Copy link
Contributor

There is no reason to do this check if speed and code size are of utmost importance.

This code is only executed at compile time, but I agree that it's unnecessary.

@CaseyCarter CaseyCarter added the enhancement Something can be improved label Jul 29, 2021
@StephanTLavavej StephanTLavavej self-assigned this Aug 5, 2021
@AreaZR
Copy link
Contributor Author

AreaZR commented Aug 5, 2021

On second thought, since this is a compile time constant, I wonder if this is a good idea.

@CaseyCarter
Copy link
Member

On second thought, since this is a compile time constant, I wonder if this is a good idea.

My intuition is that "self-assignment" at compile time is as rare as "self-assignment" at runtime and that the guard won't trigger often enough to offset the cost of evaluation. I don't think any of our supported compilers currently implements a branch predictor for constexpr evaluation, so I suspect this is compile-time guard has a larger negative impact than would a similar runtime guard. I would nonetheless be surprised to see a benchmark which demonstrated a statistically significant difference between having and not having the guard, but I'm happy to have fewer lines of code to maintain.

@AreaZR
Copy link
Contributor Author

AreaZR commented Aug 5, 2021

True. Maybe when our compiler is better.

While it is faster to just leave when the source and destination are the same, most programmers would not make that mistake, and if they did, it would be a rarity and a reflection of something going wrong in their code in the first place.

There is no reason to do this check if speed and code size are of utmost importance.
@AreaZR
Copy link
Contributor Author

AreaZR commented Aug 5, 2021

Alright I squashed everything and rebased, so this should be good to go!

@CaseyCarter
Copy link
Member

Alright I squashed everything and rebased, so this should be good to go!

It's not an issue given how small this change is, but for future reference please avoid force-pushing the branch since it interferes with incremental code review. (Merging main into your branch is a good alternative to rebasing if you need to pull new stuff into your branch, and we'll "Squash and merge" anyway when we merge.)

@AreaZR
Copy link
Contributor Author

AreaZR commented Aug 5, 2021

Alright, thank you!

@StephanTLavavej StephanTLavavej merged commit d3840f4 into microsoft:main Aug 6, 2021
@StephanTLavavej
Copy link
Member

Thanks for removing this unnecessary code! 😺 🧹 🎉

@AreaZR AreaZR deleted the patch-5 branch September 26, 2022 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Something can be improved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants