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

Codechange: [C++11] Use override specifer for overriding member declarations #7324

Merged
merged 4 commits into from Mar 24, 2019

Conversation

@M3Henry
Copy link
Contributor

M3Henry commented Mar 3, 2019

This is a C++11 feature that allows the compiler to check that a virtual
member declaration overrides a base-class member with the same signature.

Also src/blitter/32bpp_anim_sse4.hpp +38 is no longer erroneously marked
as virtual despite being a template.

@M3Henry M3Henry force-pushed the M3Henry:override-specifier branch 5 times, most recently from 4025137 to 8ccff43 Mar 3, 2019
@PeterN

This comment has been minimized.

Copy link
Member

PeterN commented Mar 4, 2019

Well done. I had started to do this myself but didn't get any near completing it.

I am not sure if the remaining /* virtual */ comments should stay or go.

The only issue I see is the Mac OS build warns on compiling objc files which include other headers:
warning: 'override' keyword is a C++11 extension [-Wc++11-extensions]

@PeterN

This comment has been minimized.

Copy link
Member

PeterN commented Mar 4, 2019

Many Window-derived classes incorrectly use virtual instead of /* virtual */. We should fix these up in separate commit(s). I believe this can be scripted easily, so I am working on this now.

I've pushed to my repo with a PR here: M3Henry#1

@M3Henry

This comment has been minimized.

Copy link
Contributor Author

M3Henry commented Mar 4, 2019

I'd say they should go.

I do remember GCC sometimes backports features and then warns that the feature is not compiler agnostic. Perhaps something similar?

@M3Henry M3Henry force-pushed the M3Henry:override-specifier branch from 9603846 to d0d6210 Mar 5, 2019
@M3Henry M3Henry force-pushed the M3Henry:override-specifier branch from d0d6210 to 424c83e Mar 23, 2019
@michicc

This comment has been minimized.

Copy link
Member

michicc commented Mar 23, 2019

There's a bunch of new useless warnings in the CI output for OSX. You could try adding the following to config.lib around line 1324:
# warning: <something> is a C++11 extension
flags="$flags -Wno-c++11-extensions"

We do want to use C++11, after all.

@M3Henry M3Henry force-pushed the M3Henry:override-specifier branch from 424c83e to 226eed3 Mar 24, 2019
@michicc

This comment has been minimized.

Copy link
Member

michicc commented Mar 24, 2019

Sorry @M3Henry, I should have said line 1323, i.e. just before the if block. Right now it's skipped for all CI clang versions.

M3Henry and others added 4 commits Mar 24, 2019
This is a C++11 feature that allows the compiler to check that a virtual
member declaration overrides a base-class member with the same signature.

Also src/blitter/32bpp_anim_sse4.hpp +38 is no longer erroneously marked
as virtual despite being a template.
@M3Henry M3Henry force-pushed the M3Henry:override-specifier branch from 226eed3 to 62ba3e6 Mar 24, 2019
Copy link
Member

michicc left a comment

Works for me. And if there's missing override somewhere, well, no harm in adding it later.

@michicc michicc merged commit 317f69c into OpenTTD:master Mar 24, 2019
8 checks passed
8 checks passed
OpenTTD CI Build #20190324.13 succeeded
Details
OpenTTD CI (Linux commit-checker) Linux commit-checker succeeded
Details
OpenTTD CI (Linux linux-amd64-clang-3.8) Linux linux-amd64-clang-3.8 succeeded
Details
OpenTTD CI (Linux linux-amd64-gcc-6) Linux linux-amd64-gcc-6 succeeded
Details
OpenTTD CI (Linux linux-i386-gcc-6) Linux linux-i386-gcc-6 succeeded
Details
OpenTTD CI (MacOS) MacOS succeeded
Details
OpenTTD CI (Windows Win32) Windows Win32 succeeded
Details
OpenTTD CI (Windows Win64) Windows Win64 succeeded
Details
@M3Henry M3Henry deleted the M3Henry:override-specifier branch Mar 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.