-
-
Notifications
You must be signed in to change notification settings - Fork 346
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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 inconst
ness, where having theoverride
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
scons build
&scons test
) and unit tests address code coverage