Compiling the code with Clang 7 results in a lot of warning messages:
$ make clean; CC=clang-7 CXX=clang++-7 make 2>&1 | grep -Eoe "\[\-W([a-z0-9A-Z]|\-)*\]" | sort | uniq -c | sort -nr
===> Cleaning up
5399 [-Winconsistent-missing-override]
18 [-Wcast-align]
8 [-Winvalid-source-encoding]
6 [-Waddress-of-packed-member]
5 [-Wunused-function]
3 [-Wreturn-std-move]
2 [-Wunused-parameter]
2 [-Wdeprecated-declarations]
1 [-Wunused-variable]
This makes it nearly impossible to distinguish 'real' warnings from noise.
Well then you can either suppress the override warning or start adding override all the places it should be. I don't think Simutrans can suppress those warnings, because that will cause unrecognized parameter errors for everybody else.
One could always disable warnings conditionally in the Makefile based on compiler and compiler version, but this would hide the underlying issue.
Attached a patch to add the missing override specifiers (gzipped to get around the file size limit).
Did you check that every new OVERRIDE statement is indeed intended?
Just adding override everywhere there is a warning could just hide problems as well. If one is to take the warnings seriously, every single one must be inspected and evaluated. With over 5000 of them, it will take time. But in this case, the elephant can be eaten a bite at a time, and with a bit of coordination, by several people in parallel.
Quote from: Dwachs on February 08, 2019, 02:32:49 PMDid you check that every new OVERRIDE statement is indeed intended?
Yes, but it would be nice if someone with more experience with the code could double check before committing.
Quote from: Ters on February 08, 2019, 04:12:55 PMWith over 5000 of them,
There are only about 280 individual locations that need adjusting, because multiple warnings may be emitted if a file is included into multiple compilation units.
this is now incorporated in r8679. I took the freedom and delete all 'virtual' declarations from functions marked as override in r8680.
Quote from: ceeac on February 08, 2019, 08:06:02 PMThere are only about 280 individual locations that need adjusting
Ah, yes. It has been so long since I did C++ programming that I forgot that most likely all of these warnings must come from header files.
Quote from: Dwachs on February 08, 2019, 09:13:23 PM
this is now incorporated in r8679. I took the freedom and delete all 'virtual' declarations from functions marked as override in r8680.
Seems reasonable, since the latter implies that the former is in effect. (Although I do have slight misgiving with the C++ standard makers that the virtuality (or is it virtualness?) of a member function can be indicated either by a keyword at the very beginning of the signature or at the very end.)
Removing virtual means that one has to abort compiling with an error, when OVERRIDE is not available, since then it is define as empty, i.e.in GCC before 4.7.
Why should there be a problem if OVERRIDE is empty?
If a basic function is only override, then after removing the override it becomes non-virtual, if the virtual is removed. It seems at least to compile without override though.
Nope:
Quote
If some member function vf is declared as virtual in a class Base, and some class Derived, which is derived, directly or indirectly, from Base, has a declaration for member function with the same
- name
- parameter type list (but not the return type)
- cv-qualifiers
- ref-qualifiers
Then this function in the class Derived is also virtual (whether or not the keyword virtual is used in its declaration)
(From https://en.cppreference.com/w/cpp/language/virtual (https://en.cppreference.com/w/cpp/language/virtual)).
If the function in the base class is not virtual, then it is only hidden by a function with the same name in the derived class, but overrride only helps in this case to make sure a derived function overrides a base function.
Basically, everywhere override does not give a compilation error, virtual is meaningless.
Yes, but we have defined override to nothing for old GCC. So no virtual and override defined to nothing would make the function non-virtual. Most of those concern the window handling, so errors may not be very obvious on first glance.
QuoteYes, but we have defined override to nothing for old GCC. So no virtual and override defined to nothing would make the function non-virtual. Most of those concern the window handling, so errors may not be very obvious on first glance.
QuoteIf a basic function is only override, then after removing the override it becomes non-virtual, if the virtual is removed. It seems at least to compile without override though.
I think there is some confusion here as to what virtual does, as Ters is trying to point out.
The base class must use the keyword virtual for the method, this has not changed. Derrived classes which implement or "override" that method do not have use use the key word virtual with it because the C++ standard states that it will implicitly override the base class implementation. That also has not changed, just before for some reason an ignored virtual was specified or those methods, possibly due to copy and pasting the prototypes from the base class. The override keyword is a modern extra tier of safety because it explicitly forces a method to implement or override a virtual method otherwise an error is generated.
In modern code both work together with the base class being virtual and all implementations of that method using override. In this configuration code errors can be spotted at compile time such as forgetting to update function declarations to reflect parameter changes or name typos causing a new method to be declared rather than overriding a virtual method.
In old code only the base class declared methods as virtual. All derived classes which implemented those virtual methods did not declare them as virtual and relied on implicit override mechanics of the C++ specification. This could allow for hard to detect runtime errors caused by code errors that are not detected at compile time such as a typo in the method name or if method parameters were changed inconsistently.
Since the only people who need to care about this are developers and developers should be using recent compilers with override keyword support, it is fine to define OVERRIDE as nothing for older compilers. The code will still build correctly on old compilers because all the errors which the override keyword catches will be detected and fixed by developers before being committed. The implicit override behaviour of virtual methods from older C++ compilers will be enough to build Simutrans correctly without the override keyword, as the override keyword only provides extra error checking and not really any additional functionality.
For details this page seems useful.
https://en.cppreference.com/w/cpp/language/virtual
Quote from: prissi on February 14, 2019, 01:38:10 PM
Yes, but we have defined override to nothing for old GCC. So no virtual and override defined to nothing would make the function non-virtual.
DrSuperGood has covered the virtual keyword, so I'll focus on override. The override keyword
requires a function to be virtual, but it
does not make a function virtual.
Quote from: prissi on February 14, 2019, 01:38:10 PM
Most of those concern the window handling, so errors may not be very obvious on first glance.
Which I believe is
exactly why the override keyword was introduced. It catches such errors. That is its only function and purpose. Java has a similar feature since 2004 (although it is not a keyword).