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

Conform to C.128, aka: Virtual functions should specify exactly one of virtual, override, or final #5995

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fluffyfreak
Copy link
Contributor

C.128: Virtual functions should specify exactly one of virtual, override, or final
I found out that there's an isocpp standard for this and decided that I'd see how much work it'd be to make us compliant with this and maybe shake out some bugs.

No issues found is the good news and after a LOT of edited files I don't think there's any cases of virtual, override, and/or final appearing in the same function declaration 👍

Sadly this does touch on a LOT of files.

@sturnclaw
Copy link
Member

I really appreciate moving to significantly less keyword noise, but this rebase does indeeed touche a number of files - and will likely cause a bunch of merge conflicts for open PRs. In that light, I'd like to merge this significantly closer to the upcoming release as a "cleanup task" once the major feature branches are done being merged.

@ollobrains

This comment was marked as off-topic.

@sturnclaw
Copy link
Member

Hi @ollobrains - I appreciate the desire to help, but I'd ask you to refrain from pasting AI-generated responses. Unfortunately, though it may look useful on paper, the only thing that the AI is doing is summarizing existing work that's already been done in the pull request and proposing it as though it needs to be done again.

@sturnclaw
Copy link
Member

It looks like there's a small merge conflict with the changes to the Missile code here. If you can fix that up, I'd love to merge this ASAP.

@fluffyfreak
Copy link
Contributor Author

I'll wait to fix this up once the Geosphere stuff is merged because there's some overlap in the headers

@sturnclaw
Copy link
Member

Geosphere stuff is merged if you're around to rebase. I'm likely going to close the merge window for anything non-critical/non-bugfix in +12 hours; this might be deferred to the start of next release cycle if not updated by then.

C.128: Virtual functions should specify exactly one of virtual, override, or final
@fluffyfreak fluffyfreak force-pushed the either-virtual-override-or-final-C-128 branch from 532710a to 330ef85 Compare February 2, 2025 13:21
@fluffyfreak
Copy link
Contributor Author

Rebased, I took the Player.h from master because it contained the correct changes I think already 👍

@fluffyfreak
Copy link
Contributor Author

Obviously this can all wait until later, I wasn't free yesterday evening to fix it up in time

Copy link
Member

@sturnclaw sturnclaw left a comment

Choose a reason for hiding this comment

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

Since this is non-critical for the release, I'm going to defer it +1 day until after we cut 20250203. Otherwise, I'd pull the trigger right now - thanks for the work on this!

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.

3 participants