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

Refactor[BMQ,MQB]: remove redundant virtual, s_allocator_p #538

Merged
merged 2 commits into from
Dec 5, 2024

Conversation

678098
Copy link
Collaborator

@678098 678098 commented Dec 5, 2024

  1. Remove s_allocator_p mentions from the code samples.
  2. Put keyword final where it was missed.
  3. Remove redundant virtual on methods that already marked as override.
    See also: https://learn.microsoft.com/en-us/cpp/code-quality/c26435?view=msvc-170
    https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#c128-virtual-functions-should-specify-exactly-one-of-virtual-override-or-final

@678098 678098 requested a review from a team as a code owner December 5, 2024 21:41
@678098 678098 requested a review from pniedzielski December 5, 2024 21:42
Copy link
Collaborator

@pniedzielski pniedzielski left a comment

Choose a reason for hiding this comment

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

For the first patch, I have no strong opinions either way (with or without virtuals in the derived classes). But verified that these virtuals are redundant.

Second patch, 👍🏻 good changes

Copy link

@bmq-oss-ci bmq-oss-ci bot left a comment

Choose a reason for hiding this comment

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

Build 399 of commit dc711ed has completed with FAILURE

@678098 678098 merged commit 35451ff into bloomberg:main Dec 5, 2024
35 checks passed
@678098 678098 deleted the 241205_cleanup branch December 5, 2024 23:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants