Make use of 'override' specifier #1589
Conversation
Codecov Report
@@ 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
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Thanks, @speth for another significant cleanup: I believe it is definitely worth the effort, as I noticed several instances where override behavior may have deviated from the original intent.
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 also noticed a change of behavior in one implementation, which does make sense, but should probably be documented a little better.
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. |
There was a problem hiding this comment.
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.
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 |
Changes proposed in this pull request
Make use of the override specifier for overrides of virtual functions. That is, in derived classes, write
instead of
where the
virtualkeyword 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 inconstness, where having theoverridespecifier will generate an error.Since the default C++11 behavior is that the
overridespecifier 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-Werrorto treat such warnings as errors.Checklist
scons build&scons test) and unit tests address code coverage