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: Use override instead of virtual where possible #4917

Merged
merged 57 commits into from
Oct 25, 2023

Conversation

pajlada
Copy link
Member

@pajlada pajlada commented Oct 24, 2023

Description

This PR fixes most warnings from the clang-tidy cppcoreguidelines-explicit-virtualfunctions check

  1. For base classes implementing virtual functions, just use virtual
  2. For derived classes overriding a base class virtual function, just use override since override implies virtual

Part of #4915

Unrelated change(s)

Irc2: Add some newlines to let the anon namespace breathe
LambdaRunnable: Initialize action_ in member initializer list
IrcChannel: Made the class final
TwitchChannel: Made the class final

How to review

Most changes are just removing virtual & the reformatting that comes with it.
TwitchChannel had 3 virtual functions that were never overwritten, refresh{BTTV,FFZ,SevenTV}ChannelEmotes. I removed virtual complete there.
Made LambdaRunnable's run function override - it was not override or virtual before, but run is most likely meant to override QRunnable's run function.
Made GeneralPage's filterElements function override
ComboBoxItemDelegate now uses override instead of virtual, since they override functions from QStyledItemDelegate

@pajlada
Copy link
Member Author

pajlada commented Oct 25, 2023

I have looked through all changes in this PR and they look good to me - I'll leave it up for a bit to give people time to look through it before I merge it in.

image

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

There were too many comments to post at once. Showing the first 25 out of 30. Check the log or trigger a new build to see more.

src/providers/twitch/TwitchChannel.hpp Show resolved Hide resolved
src/widgets/AttachedWindow.hpp Show resolved Hide resolved
src/widgets/BaseWidget.hpp Show resolved Hide resolved
src/widgets/BaseWidget.hpp Show resolved Hide resolved
src/widgets/helper/NotebookButton.hpp Show resolved Hide resolved
src/widgets/helper/NotebookButton.hpp Show resolved Hide resolved
src/widgets/helper/NotebookButton.hpp Show resolved Hide resolved
src/widgets/helper/NotebookButton.hpp Show resolved Hide resolved
src/widgets/helper/NotebookButton.hpp Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

src/widgets/helper/NotebookButton.hpp Show resolved Hide resolved
src/widgets/helper/NotebookButton.hpp Show resolved Hide resolved
src/widgets/helper/NotebookTab.hpp Show resolved Hide resolved
src/widgets/helper/NotebookTab.hpp Show resolved Hide resolved
src/widgets/helper/SettingsDialogTab.hpp Show resolved Hide resolved
@pajlada pajlada merged commit 5c0219c into master Oct 25, 2023
16 of 17 checks passed
@pajlada pajlada deleted the refactor/clang-tidy-virtual-override branch October 25, 2023 16:13
@pajlada pajlada mentioned this pull request Oct 25, 2023
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant