Use override keyword were necessary
#948
Conversation
guitargeek
changed the title
override keyword were necessaryoverride keyword were necessary
guitargeek
force-pushed
the
override
branch
2 times, most recently
from
7e85616
to
667920e
Compare
|
@guitargeek could you please update this PR with the latest main to include the new workflow added with this #946 ? |
To avoid future mistakes when overriding functions, this commit suggests to use the `override` keyword as suggested by clang-tidy. How this commit was produced: 1. Compile with CMake, exporting the compile commands as described here: https://stackoverflow.com/questions/20059670/how-to-use-cmake-export-compile-commands 2. Build 3. Run this in the build directory: ``` run-clang-tidy -header-filter="interface/.*" -config-file=../.clang-tidy -export-fixes fixes.yaml . -j20 ``` 4. Copy the `fixes.yaml` back to the main repo and apply fixes with: ``` clang-apply-replacements . ``` 5. Replace `ClassDef` macros with `ClassDefOverride` to be consistent.
Follows up on `91b791a`, making sure that only the right overload is implemented. Also, apply the same fix to the `CachingAddNLL` class.
|
Sure! I updated the PR. Thanks so much for taking care of it, @anigamova! |
|
I have updated all my other PRs as well: All these PRs are only technical but necessary for ROOT compatibility, so it would be nice if you can have a look at the others too 👍 |
interface/RooSimultaneousOpt.h
Outdated
|
|
||
| virtual ~RooSimultaneousOpt() ; | ||
| ~RooSimultaneousOpt() override ; | ||
|
|
||
| virtual RooAbsReal* createNLL(RooAbsData& data, const RooLinkedList& cmdList) ; |
There was a problem hiding this comment.
From the compilation output with ROOT 6.22 it looks like this method should also have an override.
I guess it was not automatically added because you ran clang-tidy with a ROOT version > 6.22. Could you please check?
There was a problem hiding this comment.
Indeed, but that depends on the root version. I will fix this in a separate commit/PR:
#949
You want me to add that commit also in this PR? Would that make things easier for you?
There was a problem hiding this comment.
for this particular method I think it is fine to add it in another PR.
But I just wonder how to make sure that we stay consistent and backward compatible with these override statements. For now it would be great if you could check with 6.22 and 6.26 if it is not difficult, thanks!
There was a problem hiding this comment.
I have combined the PRs after all in this one, that made it easier for me to validate.
I compiled this PR now with 6.22, 6.26, and 6.30 to check that there are no cases in which is get warnings related to overrides. And it is not the case!
To avoid future mistakes when overriding functions, this commit suggests to use the
overridekeyword as suggested by clang-tidy.How this commit was produced:
Compile with CMake, exporting the compile commands as described here: https://stackoverflow.com/questions/20059670/how-to-use-cmake-export-compile-commands
Build
Run this in the build directory:
run-clang-tidy -header-filter="interface/.*" -config-file=../.clang-tidy -export-fixes fixes.yaml . -j20Copy the
fixes.yamlback to the main repo and apply fixes with:clang-apply-replacements .