[ci] Fail CI if missing override instead of just warning #19092
Conversation
|
@pcanal I see a |
Test Results 19 files 19 suites 3d 5h 18m 58s ⏱️ For more details on these failures, see this check. Results for commit 0c8d97e. ♻️ This comment has been updated with latest results. |
|
Yes, this is also my observation. |
|
Thanks @linev for the feedback. Btw, do you approve the changes in roottest/io ? |
| @@ -6,10 +6,5 @@ class RtObj : public T | |||
| public: | |||
| RtObj() {} | |||
| RtObj( const T & val ) : T(val) {} | |||
| ClassDefT(RtObj,1) | |||
| ClassDefOverride(RtObj,1) | |||
There was a problem hiding this comment.
Why using override here?
None of virtual method is override.
Also not sure if ClassDefT is equivalent to ClassDefOverride
There was a problem hiding this comment.
ClassDefT is deprecated. It contains exactly the same definition as ClassDef.
There was a problem hiding this comment.
Wrt override: same explanation, if you derive from TObject, you need to use ClassDefOverride instead of ClassDef.
| @@ -4,7 +4,7 @@ class A:public TObject{ | |||
|
|
|||
| public: | |||
| A(){} | |||
| ClassDef(A,1) | |||
| ClassDefOverride(A,1) | |||
There was a problem hiding this comment.
Same here - as long there are no new virtual methods override, there is no sense to use ClassDefOverride
There was a problem hiding this comment.
It derives from TObject, which has itself a ClassDef with virtual functions.
within the derived class, if you define ClassDef again, it redefines those virtual.
So you need ClassDefOverride.
For this part clearly Philippe @pcanal is responsible |
Currently, many override warnings remain unnoticed since the gcc-problem-matcher of the GitHub CI does not work correctly, and it's also easy to forget those annotations if you do not visit the "Files changed" tab.
If some devs later use dev=ON and testing=ON, this leads to failures for compilers that auto-enable -Winconsistent-missing-override.
To prevent this, we enable the suggest-override warning in the build of debian with dev=ON instead of in the one from alma, this way we are sure of always failing if some does a PR that forgets an override, forcing pull-requesters to fix their issues before merging.
Within this PR: also fix missing overrides uncovered by turning ON this flag in the dev=ON build.