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

Update list to remove bogus strengthened comment #5024

Merged
merged 4 commits into from
Oct 21, 2024
Merged

Update list to remove bogus strengthened comment #5024

merged 4 commits into from
Oct 21, 2024

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

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

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
stl/inc/list Outdated Show resolved Hide resolved
@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

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

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

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

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

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
39 checks passed
@StephanTLavavej
Copy link
Member

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

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

@StephanTLavavej StephanTLavavej StephanTLavavej approved these changes

Assignees

@StephanTLavavej StephanTLavavej

Labels
documentation Related to documentation or comments
Projects
STL Code Reviews
Archived in project
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