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

Optimize <xstring> memmove function #2055

Merged
merged 26 commits into from
Jul 20, 2021
Merged

Conversation

AZero13
Copy link
Contributor

@AZero13 AZero13 commented Jul 10, 2021

Optimized memmove

@AZero13 AZero13 requested a review from a team as a code owner July 10, 2021 17:10
@fsb4000
Copy link
Contributor

fsb4000 commented Jul 10, 2021

CI errors:

D:\build\out\inc\cstring(26,13): error: no member named 'memccpy' in the global namespace

using _CSTD memccpy;

      ~~~~~ ^

D:\build\out\inc\cstring(38,13): error: no member named 'strdup' in the global namespace; did you mean '_strdup'?

using _CSTD strdup;

      ~~~~~ ^~~~~~

            _strdup

C:\Program Files (x86)\Windows Kits\10\include\10.0.19041.0\ucrt\string.h(148,38): note: '_strdup' declared here

_ACRTIMP _CRTALLOCATOR char* __cdecl _strdup(

                                     ^

2 errors generated.

P.S. I doubt your PR will be accepted because Non-goal: Adding non-Standard extensions.
But I am not a maintainer of this repository so I could be wrong.
You will have a better chance that your PR will be accepted if you solve some existing issue.
Check out https://github.com/microsoft/STL/issues

@AZero13 AZero13 changed the title Add more c string functions to cstring Add strnlen to cstring Jul 10, 2021
@AZero13 AZero13 changed the title Add strnlen to cstring Add strnlen to cstring and optimize xstring Jul 11, 2021
@miscco
Copy link
Contributor

miscco commented Jul 12, 2021

I do not see any functional code change in product code. Please revert purely aesthetical changes

@AZero13
Copy link
Contributor Author

AZero13 commented Jul 12, 2021

I do not see any functional code change in product code. Please revert purely aesthetical changes

Done

stl/inc/xstring Outdated Show resolved Hide resolved
@StephanTLavavej StephanTLavavej added the enhancement Something can be improved label Jul 13, 2021
stl/inc/cstring Outdated Show resolved Hide resolved
stl/inc/xstring Outdated Show resolved Hide resolved
@AZero13 AZero13 changed the title Add strnlen to cstring and optimize xstring Optimize xstring memmove function Jul 13, 2021
@CaseyCarter CaseyCarter changed the title Optimize xstring memmove function Optimize <xstring> memmove function Jul 14, 2021
Gives the code a more consistent feel
@StephanTLavavej StephanTLavavej self-assigned this Jul 19, 2021
@StephanTLavavej StephanTLavavej merged commit bd7adb4 into microsoft:main Jul 20, 2021
@StephanTLavavej
Copy link
Member

StephanTLavavej commented Jul 20, 2021

Thanks for this simplification, and congratulations on your first microsoft/STL commit! 🚀 😺 This will ship in VS 2022 17.0 Preview 4.

A note for the future: when pushing changes after reviewers have approved, it's a good idea to add a comment to the PR mentioning that you've done so, so the reviewers know to take another look. (Some changes that seem obviously desirable can be inconsistent with a coding convention, or can have surprising interactions with language complexity, etc. - which is why we're so careful about getting all changes to be seen by multiple reviewers.) Here, Casey and I had approved and moved this to Ready To Merge, and I wasn't looking closely for unannounced changes, so your last commit caught me by surprise. Fortunately Casey noticed and re-approved, so all was well (and I'm also fine with the change). This also happened before I mirrored the GitHub PR to the MSVC-internal repo, so no changes were dropped on the floor. I've now realized that we need to document these expectations better, so I'll add this to the Contribution Guidelines document that I need to write up.

@AZero13 AZero13 deleted the patch-1 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.

8 participants