Skip to content

Update list to remove bogus strengthened comment#5024

Merged
StephanTLavavej merged 4 commits intomicrosoft:mainfrom
5AIPAVAN:main
Oct 21, 2024
Merged

Update list to remove bogus strengthened comment#5024
StephanTLavavej merged 4 commits intomicrosoft:mainfrom
5AIPAVAN:main

Conversation

@5AIPAVAN
Copy link
Contributor

@5AIPAVAN 5AIPAVAN commented Oct 17, 2024 •
edited by StephanTLavavej
Loading

Fixes #5020.

@5AIPAVAN 5AIPAVAN requested a review from a team as a code owner October 17, 2024 09:13
@AlexGuteniev
Copy link
Contributor

AlexGuteniev commented Oct 17, 2024

clang format validation failed. run clang-format (from IDE or standalone) and update

@StephanTLavavej StephanTLavavej added the documentation Related to documentation or comments label Oct 17, 2024
@StephanTLavavej
Copy link
Member

StephanTLavavej commented Oct 17, 2024

Additionally:

  • Please edit your PR title to describe (very briefly) what your change is doing - mentioning issue numbers there is not useful. PR titles become part of git history (e.g. see git log or the graphical gitk), and a long series of "solve issue #nnnn" is completely opaque. "Update list to remove bogus strengthened comment" would be perfectly descriptive, or you can phrase it another way.
  • Please edit your PR description to use GitHub's fix/close/resolve syntax so that your PR is properly linked to the issue that it resolves. "Fixes #nnnn", "Resolves #nnnn", or "Closes #nnnn" are all fine.

Thanks!

frederick-vs-ja
frederick-vs-ja reviewed Oct 18, 2024
View reviewed changes
@5AIPAVAN 5AIPAVAN changed the title Update list to solve issue #5020 Removed /* strengthened */ in <list> line 924 Oct 18, 2024
@5AIPAVAN 5AIPAVAN changed the title Removed /* strengthened */ in <list> line 924 Update list to remove bogus strengthened comment Oct 18, 2024
@5AIPAVAN
Update xtree
d149a58 
replaced
_Choose_pocma_v<_Alnode> == _Pocma_values::_Equal_allocators
with
_Alnode_traits::is_always_equal::value
@5AIPAVAN
Update list
4db39e6 
changed
Choose_pocma_v<_Alnode> == _Pocma_values::_Equal_allocators
to
_Alnode_traits::is_always_equal::value in <list> line 923
@AlexGuteniev
Copy link
Contributor

AlexGuteniev commented Oct 18, 2024

You still haven't ran clang-format.

If you are trying to do commit using web interface only then I want to tell that this is generally not a path forward.
Whereas you may be able to finish this change this way, it doesn't let you get any further, when you have to test and validate your changes locally. I recommend you learning how to use a Git/Github client and following README.md steps to build this locally.

(I admit I have done some web commits on my own, but the purpose of good first issue is for a contributor to learn the workflow; using web interface defeats this purpose)

@5AIPAVAN
Copy link
Contributor Author

5AIPAVAN commented Oct 18, 2024

You still haven't ran clang-format.

If you are trying to do commit using web interface only then I want to tell that this is generally not a path forward. Whereas you may be able to finish this change this way, it doesn't let you get any further, when you have to test and validate your changes locally. I recommend you learning how to use a Git/Github client and following README.md steps to build this locally.

(I admit I have done some web commits on my own, but the purpose of good first issue is for a contributor to learn the workflow; using web interface defeats this purpose)

sure sir, actually i tried with some online clang formatters(they have shown that code is formatted correctly) and some vs code extensions, but haven't looked in depth
will try now and update by end of the day
thanks for your suggesions :-)

@5AIPAVAN
Copy link
Contributor Author

5AIPAVAN commented Oct 18, 2024

runned it locally
cformat

frederick-vs-ja
frederick-vs-ja approved these changes Oct 18, 2024
View reviewed changes
Copy link
Contributor

@frederick-vs-ja frederick-vs-ja left a comment

Choose a reason for hiding this comment

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

LGTM assuming PR message fixed.

@StephanTLavavej
Copy link
Member

StephanTLavavej commented Oct 18, 2024

Thanks! I went ahead and edited your PR description to use GitHub's fix/close/resolve syntax.

This PR will be part of our next merge batch, likely next week. I'll post comments as I prepare your PRs for merging.

@StephanTLavavej StephanTLavavej self-assigned this Oct 21, 2024
@StephanTLavavej
Copy link
Member

StephanTLavavej commented Oct 21, 2024

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

@StephanTLavavej StephanTLavavej merged commit 7fe4057 into microsoft:main Oct 21, 2024
@StephanTLavavej
Copy link
Member

StephanTLavavej commented Oct 21, 2024

Thanks for the fix, and congratulations on your first microsoft/STL commit! 🎉 🐈 😸

This change is expected to ship in VS 2022 17.13 Preview 2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@StephanTLavavej StephanTLavavej StephanTLavavej approved these changes

+1 more reviewer

@frederick-vs-ja frederick-vs-ja frederick-vs-ja approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@StephanTLavavej StephanTLavavej

Labels

documentation Related to documentation or comments

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

<list>: Remove an incorrect /* strengthened */ comment from the move assignment operator

4 participants

@5AIPAVAN @AlexGuteniev @StephanTLavavej @frederick-vs-ja