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.

Sign up for GitHub

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

Jump to bottom

Fix bugs in std::string #2305

Merged
merged 8 commits into from
Dec 17, 2021
Merged

Fix bugs in std::string #2305

merged 8 commits into from
Dec 17, 2021

Conversation

miscco
Copy link
Contributor

@miscco miscco commented Oct 26, 2021 •
edited by CaseyCarter
Loading

This pulls out the bugfixes for std::string from #1735

  • Missing capacity for null terminator
  • Leaking memory in case of unequal allocators
  • Not swapping the proxy during swap
  • swapping data in case of self swap

I did keep the _Is_element_ptr changes, please bear with me here

@miscco
Fix bugs in std::string
c86e19b 
* Missing capacity for null terminator
* Leaking memor in case of unequal allocators
* Not swapping the proxy during swap
* swapping data in case of self swap
@miscco miscco requested a review from a team as a code owner October 26, 2021 19:12
@CaseyCarter CaseyCarter added the bug Something isn't working label Oct 26, 2021
AlexGuteniev
AlexGuteniev reviewed Oct 26, 2021
View reviewed changes
stl/inc/xstring Outdated Show resolved Hide resolved
CaseyCarter
CaseyCarter previously requested changes Oct 27, 2021
View reviewed changes
stl/inc/xstring Outdated Show resolved Hide resolved
tests/std/tests/P0980R1_constexpr_strings/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/P0980R1_constexpr_strings/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/P0980R1_constexpr_strings/test.cpp Outdated Show resolved Hide resolved
@mnatsuhara mnatsuhara self-assigned this Nov 17, 2021
mnatsuhara
mnatsuhara reviewed Nov 29, 2021
View reviewed changes
Copy link
Contributor

@mnatsuhara mnatsuhara left a comment

Choose a reason for hiding this comment

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

Reviewed all changes and they look good! I'm really happy with the additional test coverage -- thanks for going through and adding all of that :) Just one question regarding test coverage for EDG.

tests/std/tests/P0980R1_constexpr_strings/test.cpp Outdated Show resolved Hide resolved
@mnatsuhara mnatsuhara removed their assignment Nov 30, 2021
@StephanTLavavej StephanTLavavej self-assigned this Dec 1, 2021
@CaseyCarter CaseyCarter self-requested a review December 8, 2021 01:56
@StephanTLavavej
Merge branch 'main' into fix_string
7353f56
@StephanTLavavej
Fine-grained EDG workarounds.
7d0989c 
Also change the MSVC internal testing guard to EDG.
(As an aside, that internal macro was recently underscore-prefixed.)
@StephanTLavavej
After GH 2064, we guard is_constant_evaluated with _HAS_CXX20.
617caeb
@StephanTLavavej
Cleanup: Use _My_data/_Right_data, inline _My_large/_Right_large.
f8c3c32
@StephanTLavavej
Revert _Is_elem_cptr_v due to /clr:pure failures.
68d7023 
test.obj : fatal error LNK1179: invalid or corrupt file: duplicate COMDAT '???__E??$_Is_elem_cptr_v@QB_W@?$basic_string@_WU?$char_traits@_W@std@@v?$allocator@_W@2@@std@@$$Q0_NB@?$basic_string@_WU?$char_traits@_W@std@@v?$allocator@_W@2@@std@@YMXXZ@?A0xd54b7a0b@@$$FYMXXZ'
@StephanTLavavej
Copy link
Member

I've pushed the following changes:

  • Merge main, conflict-free.
  • Fine-grained EDG workarounds, as requested by @mnatsuhara. Some are debug-only.
    • These workarounds were almost verbose enough to abandon the attempt, but Miya made an excellent case for preventing regressions in brittle compiler codepaths and that's worth the effort.
    • Also, this replaces the "MSVC internal testing" guard with an EDG guard. The affected code works (or does a good job of appearing to work) with both the released VS Preview and internal builds of MSVC, but doesn't work for EDG.
    • As an aside, the "MSVC internal testing" guard was recently renamed to add an underscore: _MSVC_INTERNAL_TESTING
  • After Toolset update: VS 2022 17.0 Preview 2 #2064 was merged, we guard is_constant_evaluated with _HAS_CXX20.
  • Cleanup: use _My_data/_Right_data to avoid verbosity. Now that _My_large/_Right_large aren't needed in multiple places, "inline" them with no loss of clarity.
  • The _Is_elem_cptr_v change was desirable, but must be reverted due to /clr:pure failures of the form test.obj : fatal error LNK1179: invalid or corrupt file: duplicate COMDAT '???__E??$_Is_elem_cptr_v@[...blah...]'. (I believe it dislikes variable templates other than at namespace scope.)

Thanks for fixing all these bugs, especially the one where swap didn't think it needed to swap proxies for the small-small case. I was sure that change wasn't necessary, until I attempted to revert it and discovered a test failure, and then I was sure the test was bogus, until I looked further and saw that the test was quite strict about the behavior of equality. Nicely done!

@StephanTLavavej StephanTLavavej removed their assignment Dec 15, 2021
@StephanTLavavej StephanTLavavej dismissed CaseyCarter’s stale review December 15, 2021 10:55

Verified that all of Casey's comments have been addressed.

@StephanTLavavej StephanTLavavej self-assigned this Dec 16, 2021
@StephanTLavavej
Copy link
Member

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej merged commit 5eb9cb8 into microsoft:main Dec 17, 2021
@StephanTLavavej
Copy link
Member

Thanks 🐞 for 🐜 fixing 🐛 these 🪲 bugs! 🐝

@miscco miscco deleted the fix_string branch December 17, 2021 09:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlexGuteniev AlexGuteniev AlexGuteniev left review comments

@mnatsuhara mnatsuhara mnatsuhara approved these changes

@StephanTLavavej StephanTLavavej StephanTLavavej approved these changes

@CaseyCarter CaseyCarter Awaiting requested review from CaseyCarter

Assignees

@StephanTLavavej StephanTLavavej

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@miscco @StephanTLavavej @CaseyCarter @AlexGuteniev @mnatsuhara