Make use of 'override' specifier

[speth]

Changes proposed in this pull request

Make use of the override specifier for overrides of virtual functions. That is, in derived classes, write

int foo(double bar) override;

instead of

virtual foo(double bar);

where the virtual keyword in the derived class actually has no effect. This helps distinguish between virtual functions introduced in a class and overrides of base class methods. More importantly, it helps detect some subtle errors, where an override is desired but does not actually correspond to the base class method, for example by having a different argument type or a difference in constness, where having the override specifier will generate an error.

Since the default C++11 behavior is that the override specifier is not required (for backwards compatibility), this PR also updates the Clang CI runner to issue a warning in cases where it could be used, and is further configured with -Werror to treat such warnings as errors.

Checklist

• [x] The pull request includes a clear description of this code change
• [x] Commit messages have short titles and reference relevant issues
• [x] Build passes (scons build & scons test) and unit tests address code coverage
• [x] Style & formatting of contributed code follows contributing guidelines
• [x] The pull request is ready for review

[codecov[bot]]

Codecov Report

Merging #1589 (e7862d2) into main (717af98) will increase coverage by 0.01%. The diff coverage is 81.10%.

@@            Coverage Diff             @@
##             main    #1589      +/-   ##
==========================================
+ Coverage   70.58%   70.60%   +0.01%     
==========================================
  Files         379      379              
  Lines       59164    59153      -11     
  Branches    21263    21252      -11     
==========================================
+ Hits        41763    41766       +3     
+ Misses      14327    14313      -14     
  Partials     3074     3074              

Files Changed	Coverage Δ
include/cantera/base/Array.h	96.00% <ø> (ø)
include/cantera/base/ExtensionManagerFactory.h	100.00% <ø> (ø)
include/cantera/base/NoExitLogger.h	0.00% <0.00%> (ø)
include/cantera/kinetics/Custom.h	100.00% <ø> (ø)
include/cantera/numerics/AdaptivePreconditioner.h	96.29% <ø> (ø)
include/cantera/numerics/BandMatrix.h	0.00% <ø> (ø)
include/cantera/numerics/DenseMatrix.h	100.00% <ø> (ø)
include/cantera/oneD/DomainFactory.h	72.72% <ø> (ø)
include/cantera/oneD/IonFlow.h	73.33% <0.00%> (ø)
include/cantera/oneD/Sim1D.h	66.66% <ø> (ø)
... and 94 more

... and 1 file with indirect coverage changes

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

[speth]

I did, however, notice that you removed several comments. While they may be indeed redundant, I'd prefer to keep them a little longer as there is little harm, and we're not really making inroads on Cantera/enhancements#6 yet. Edit: in case the comments are indeed exact replicas, then I'm ok with removal.

I think redundant documentation does do harm: it gives the illusion that there is any meaningful documentation for what these methods do, reducing the impetus to make improvements. While doing a broad scan for this is pretty tedious, I thought it was worth cleaning up a few instances while I was looking at these methods and their base class analogues.

[speth]

I don't disagree. Not being too familiar with the ThermoPhase side of things, I still appreciate confirmations here, as there were no comments in the commit history. Sorry for trying to double-check, but once a comment is removed, it's hard to go back.

No worries. I appreciate the attention to detail. Sorry for not breaking these out in the commit history to make the intention and rationale more clear; I just didn't want to get into a merge conflict fight within my own PR, which all of these would have triggered since they're on adjacent lines to the override changes.