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

Remove redundant uses of virtual keyword #2069

Merged
merged 6 commits into from
Aug 6, 2021
Merged

Remove redundant uses of virtual keyword #2069

merged 6 commits into from
Aug 6, 2021

Conversation

SuperWig
Copy link
Contributor

@SuperWig SuperWig commented Jul 21, 2021 •
edited
Loading

Fixes #303.
Fixes #207.

Opening as draft due to "decision-needed" tag, though there wasn't anyone opposing. The ones that go onto a new line do make it harder to notice the override.

AlexGuteniev
AlexGuteniev reviewed Jul 21, 2021
View reviewed changes
stl/inc/cvt/jis_0208 Outdated Show resolved Hide resolved
@CaseyCarter CaseyCarter added the enhancement Something can be improved label Jul 21, 2021
@StephanTLavavej
Copy link
Member

We've decided that this is indeed desirable - thanks! 😸

@AlexGuteniev
Copy link
Contributor

Apparently this will resolve #207 as well

@SuperWig SuperWig marked this pull request as ready for review July 22, 2021 07:44
@SuperWig SuperWig requested a review from a team as a code owner July 22, 2021 07:44
@StephanTLavavej StephanTLavavej mentioned this pull request Jul 23, 2021
Merged
@StephanTLavavej StephanTLavavej self-assigned this Jul 28, 2021
@StephanTLavavej
Copy link
Member

Looks great, thanks! I've pushed a merge with main (no conflicts, including stealth clang-format conflicts) and I found no additional occurrences in product code that need to be changed. The fact that we were missing virtual entirely in some locations was 🙀 - thank you for cleaning everything up to say override. 😻

@StephanTLavavej StephanTLavavej removed their assignment Aug 5, 2021
@StephanTLavavej StephanTLavavej self-assigned this Aug 5, 2021
@AlexGuteniev AlexGuteniev mentioned this pull request Aug 5, 2021
Open
@StephanTLavavej StephanTLavavej merged commit c76042b into microsoft:main Aug 6, 2021
@StephanTLavavej
Copy link
Member

Thanks for this comprehensive cleanup! 🧹 😻 🚀

@SuperWig SuperWig deleted the override branch August 6, 2021 06:53
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

@CaseyCarter CaseyCarter CaseyCarter approved these changes

@StephanTLavavej StephanTLavavej StephanTLavavej approved these changes

Assignees

@StephanTLavavej StephanTLavavej

Labels
enhancement Something can be improved
Projects
None yet
Milestone
No milestone
4 participants
@SuperWig @StephanTLavavej @AlexGuteniev @CaseyCarter