[ci] use override keyword and build with Wsuggestoverride #18918

ferdymercury · 2025-05-30T07:43:25Z

This Pull request:

Changes or fixes:

Fixes https://its.cern.ch/jira/browse/ROOT-8302

fyi @wlav

github-actions · 2025-05-30T13:39:30Z

Test Results

19 files 19 suites 3d 10h 19m 27s ⏱️
2 810 tests 2 810 ✅ 0 💤 0 ❌
51 894 runs 51 894 ✅ 0 💤 0 ❌

Results for commit 6457831.

♻️ This comment has been updated with latest results.

[ci] move to alma9 as warnings

[core] missing override in classdef [test] add proper override in rqobjecttester

[net] tdavixfile apply override

ferdymercury · 2025-06-02T17:04:10Z

There are 10 warnings remainings, but they cannot be easily solved, since we get:

2025-06-02T12:13:00.6010230Z ##[warning]/Users/sftnight/ROOT-CI/src/math/mathcore/inc/Fit/Chi2FCN.h:123:9: warning: 'Gradient' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
2025-06-02T12:13:00.6011290Z    void Gradient(const double *x, double *g) const {
2025-06-02T12:13:00.6011470Z         ^
2025-06-02T12:13:00.6012270Z /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/c++/v1/__memory/unique_ptr.h:686:30: note: in instantiation of template class 'ROOT::Fit::Chi2FCN<ROOT::Math::IGradientFunctionMultiDimTempl<double>, ROOT::Math::IParametricFunctionMultiDimTempl<Vc_1::Vector<double, Vc_1::VectorAbi::Scalar>>>' requested here
2025-06-02T12:13:00.6013230Z   return unique_ptr<_Tp>(new _Tp(_VSTD::forward<_Args>(__args)...));
2025-06-02T12:13:00.6013440Z                              ^
2025-06-02T12:13:00.6014820Z /Users/sftnight/ROOT-CI/src/math/mathcore/src/Fitter.cxx:346:43: note: in instantiation of function template specialization 'std::make_unique<ROOT::Fit::Chi2FCN<ROOT::Math::IGradientFunctionMultiDimTempl<double>, ROOT::Math::IParametricFunctionMultiDimTempl<Vc_1::Vector<double, Vc_1::VectorAbi::Scalar>>>, std::shared_ptr<ROOT::Fit::BinData> &, std::shared_ptr<ROOT::Math::IParametricGradFunctionMultiDimTempl<Vc_1::Vector<double, Vc_1::VectorAbi::Scalar>>> &, const ROOT::EExecutionPolicy &>' requested here
2025-06-02T12:13:00.6016200Z                return DoMinimization(std::make_unique<Chi2FCN<BaseGradFunc, IModelFunction_v>>(data, gradFun, executionPolicy));
2025-06-02T12:13:00.6016520Z                                           ^
2025-06-02T12:13:00.6016900Z /Users/sftnight/ROOT-CI/src/math/mathcore/inc/Math/IFunction.h:179:23: note: overridden virtual function is here
2025-06-02T12:13:00.6017270Z          virtual void Gradient(const T *x, T *grad) const
2025-06-02T12:13:00.6017450Z                       ^

but if we fix it in the code, then we get an error due to a different pathway of template type overload (different base class):

  Error: /github/home/ROOT-CI/src/math/mathcore/inc/Fit/Chi2FCN.h:123:9: error: ‘void ROOT::Fit::Chi2FCN<DerivFunType, ModelFunType>::Gradient(const double*, double*) const [with DerivFunType = ROOT::Math::IBaseFunctionMultiDimTempl<double>; ModelFunType = ROOT::Math::IParametricFunctionMultiDimTempl<double>]’ marked ‘override’, but does not override
    123 |    void Gradient(const double *x, double *g) const override {
        |         ^~~~~~~~
  Error: /github/home/ROOT-CI/src/math/mathcore/inc/Fit/Chi2FCN.h:153:11: error: ‘double ROOT::Fit::Chi2FCN<DerivFunType, ModelFunType>::DoDerivative(const double*, unsigned int) const [with DerivFunType = ROOT::Math::IBaseFunctionMultiDimTempl<double>; ModelFunType = ROOT::Math::IParametricFunctionMultiDimTempl<double>]’ marked ‘override’, but does not override
      153 |    double DoDerivative(const double * x, unsigned int icoord) const override {
        |           ^~~~~~~~~~~~

since IGradientFunctionMultiDimTempl implements a virtual Gradient function and DoDerivative function, but IBaseFunctionMultiDimTempl does not.

This IBaseFunctionMultiDimTempl is called by Linkdef3.h, not sure if it can be commented out.

Co-authored-by: Philippe Canal <pcanal@fnal.gov>

pcanal

Thanks.

ferdymercury · 2025-06-13T17:54:31Z

There are 10 warnings remainings, but they cannot be easily solved, since we get:

2025-06-02T12:13:00.6010230Z ##[warning]/Users/sftnight/ROOT-CI/src/math/mathcore/inc/Fit/Chi2FCN.h:123:9: warning: 'Gradient' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
2025-06-02T12:13:00.6011290Z    void Gradient(const double *x, double *g) const {
2025-06-02T12:13:00.6011470Z         ^
2025-06-02T12:13:00.6012270Z /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/c++/v1/__memory/unique_ptr.h:686:30: note: in instantiation of template class 'ROOT::Fit::Chi2FCN<ROOT::Math::IGradientFunctionMultiDimTempl<double>, ROOT::Math::IParametricFunctionMultiDimTempl<Vc_1::Vector<double, Vc_1::VectorAbi::Scalar>>>' requested here
2025-06-02T12:13:00.6013230Z   return unique_ptr<_Tp>(new _Tp(_VSTD::forward<_Args>(__args)...));
2025-06-02T12:13:00.6013440Z                              ^
2025-06-02T12:13:00.6014820Z /Users/sftnight/ROOT-CI/src/math/mathcore/src/Fitter.cxx:346:43: note: in instantiation of function template specialization 'std::make_unique<ROOT::Fit::Chi2FCN<ROOT::Math::IGradientFunctionMultiDimTempl<double>, ROOT::Math::IParametricFunctionMultiDimTempl<Vc_1::Vector<double, Vc_1::VectorAbi::Scalar>>>, std::shared_ptr<ROOT::Fit::BinData> &, std::shared_ptr<ROOT::Math::IParametricGradFunctionMultiDimTempl<Vc_1::Vector<double, Vc_1::VectorAbi::Scalar>>> &, const ROOT::EExecutionPolicy &>' requested here
2025-06-02T12:13:00.6016200Z                return DoMinimization(std::make_unique<Chi2FCN<BaseGradFunc, IModelFunction_v>>(data, gradFun, executionPolicy));
2025-06-02T12:13:00.6016520Z                                           ^
2025-06-02T12:13:00.6016900Z /Users/sftnight/ROOT-CI/src/math/mathcore/inc/Math/IFunction.h:179:23: note: overridden virtual function is here
2025-06-02T12:13:00.6017270Z          virtual void Gradient(const T *x, T *grad) const
2025-06-02T12:13:00.6017450Z                       ^

but if we fix it in the code, then we get an error due to a different pathway of template type overload (different base class):

  Error: /github/home/ROOT-CI/src/math/mathcore/inc/Fit/Chi2FCN.h:123:9: error: ‘void ROOT::Fit::Chi2FCN<DerivFunType, ModelFunType>::Gradient(const double*, double*) const [with DerivFunType = ROOT::Math::IBaseFunctionMultiDimTempl<double>; ModelFunType = ROOT::Math::IParametricFunctionMultiDimTempl<double>]’ marked ‘override’, but does not override
    123 |    void Gradient(const double *x, double *g) const override {
        |         ^~~~~~~~
  Error: /github/home/ROOT-CI/src/math/mathcore/inc/Fit/Chi2FCN.h:153:11: error: ‘double ROOT::Fit::Chi2FCN<DerivFunType, ModelFunType>::DoDerivative(const double*, unsigned int) const [with DerivFunType = ROOT::Math::IBaseFunctionMultiDimTempl<double>; ModelFunType = ROOT::Math::IParametricFunctionMultiDimTempl<double>]’ marked ‘override’, but does not override
      153 |    double DoDerivative(const double * x, unsigned int icoord) const override {
        |           ^~~~~~~~~~~~

since IGradientFunctionMultiDimTempl implements a virtual Gradient function and DoDerivative function, but IBaseFunctionMultiDimTempl does not.

This IBaseFunctionMultiDimTempl is called by Linkdef3.h, not sure if it can be commented out.

@pcanal thanks for the feedback! Do you have a suggestion on how to deal with the 10 remaining warnings?

pcanal · 2025-06-13T18:15:20Z

If I understand correctly, this code snippet: https://godbolt.org/z/WWba5PqM5 is representing the dilemna where we would want to 'transform' o3/o4 in o1/o2 but o2 fails to compile. I have misunderstood please correct me and ignore the next paragraph.

In the case described in the code snippet, the 'only' solution I can think of is to clarify that it is expected (assuming it is indeed expected) that the base class may or may not include the Gradient virtual function and explicitly disable (by using gcc push/pop pragmas) the warning around those line of code.

[Alternatively the base class may be genuinely broken ... ]

pcanal · 2025-06-13T20:37:10Z

The error seen at https://github.com/root-project/root/pull/18918/checks?check_run_id=44076069569 is (likely) triggered by the change in the PR.

math/mathcore/inc/Fit/LogLikelihoodFCN.h

.github/workflows/root-ci.yml

math/mathcore/inc/Fit/LogLikelihoodFCN.h

math/mathcore/inc/Fit/PoissonLikelihoodFCN.h

Since override can not be consistently applied due to different base template parameters implementing or not the corresponding Gradient function

guitargeek

Thank you very much for doing this and also addressing the review comments! That's incredibly useful.

By the way, I have seem many bugs because people thought they were overriding a virtual function, but it was not virtual. Consistently using override gives you a compiler waning in these cases 👍

ferdymercury requested a review from dpiparo as a code owner May 30, 2025 07:43

ferdymercury mentioned this pull request May 30, 2025

[ci] Build alma9 with the -Wsuggest-overrides option #16364

Closed

ferdymercury changed the title ~~[ci] Test issue 8302~~ [ci] Build debian with Wsuggestoverride, Test issue 8302 May 30, 2025

ferdymercury requested a review from pcanal as a code owner May 30, 2025 08:03

bellenot assigned dpiparo May 30, 2025

ferdymercury requested a review from guitargeek May 30, 2025 11:41

ferdymercury requested a review from aaronj0 May 30, 2025 13:45

ferdymercury requested a review from vepadulano as a code owner May 30, 2025 13:52

ferdymercury requested a review from lmoneta as a code owner May 31, 2025 12:21

ferdymercury marked this pull request as draft May 31, 2025 12:25

ferdymercury changed the title ~~[ci] Build debian with Wsuggestoverride, Test issue 8302~~ [ci] Build with Wsuggestoverride, Test issue 8302 May 31, 2025

ferdymercury added 3 commits June 1, 2025 17:28

[ci] Test issue 8302

926cf03

[ci] move to alma9 as warnings

[core] missing final keywords

cc28be0

[core] missing override in classdef [test] add proper override in rqobjecttester

[pyroot] missing override keywords

5f38b64

ferdymercury force-pushed the patch-13 branch 8 times, most recently from fd42072 to 7833f3b Compare June 2, 2025 08:08

ferdymercury changed the title ~~[ci] Build with Wsuggestoverride, Test issue 8302~~ [ci] use override keyword and build with Wsuggestoverride Jun 2, 2025

ferdymercury force-pushed the patch-13 branch from 7833f3b to 57e0ab4 Compare June 2, 2025 08:36

ferdymercury added clean build and removed clean build labels Jun 2, 2025

ferdymercury added 2 commits June 2, 2025 13:36

[io] use Override in MakeProject if TObject derived

c105c6f

[net] override from TFile interface

c15742b

[net] tdavixfile apply override

ferdymercury force-pushed the patch-13 branch from e8bacb9 to 6d87d00 Compare June 2, 2025 11:59

ferdymercury added the clean build label Jun 2, 2025

ferdymercury closed this Jun 2, 2025

ferdymercury reopened this Jun 2, 2025

ferdymercury marked this pull request as ready for review June 2, 2025 18:11

ferdymercury requested review from jblomer and martamaja10 as code owners June 2, 2025 18:11

ferdymercury requested a review from silverweed June 2, 2025 21:21

ferdymercury mentioned this pull request Jun 4, 2025

Mention ClassDefOverride root-project/web#1118

Open

ferdymercury requested a review from pcanal June 11, 2025 21:51

[io][nfc] add explanation on rationale of IsTObject check

9525e31

Co-authored-by: Philippe Canal <pcanal@fnal.gov>

pcanal approved these changes Jun 13, 2025

View reviewed changes

pcanal closed this Jun 13, 2025

pcanal reopened this Jun 13, 2025

pcanal self-requested a review June 13, 2025 18:15

[roottest] add missing override

96c17a9

guitargeek reviewed Jun 14, 2025

View reviewed changes

math/mathcore/inc/Fit/LogLikelihoodFCN.h Outdated Show resolved Hide resolved

guitargeek reviewed Jun 14, 2025

View reviewed changes

.github/workflows/root-ci.yml Show resolved Hide resolved

ferdymercury commented Jun 14, 2025

View reviewed changes

math/mathcore/inc/Fit/LogLikelihoodFCN.h Outdated Show resolved Hide resolved

ferdymercury commented Jun 14, 2025

View reviewed changes

math/mathcore/inc/Fit/LogLikelihoodFCN.h Outdated Show resolved Hide resolved

ferdymercury commented Jun 14, 2025

View reviewed changes

math/mathcore/inc/Fit/PoissonLikelihoodFCN.h Outdated Show resolved Hide resolved

ferdymercury commented Jun 14, 2025

View reviewed changes

math/mathcore/inc/Fit/PoissonLikelihoodFCN.h Outdated Show resolved Hide resolved

[mathcore] rescue virtual functions

6457831

Since override can not be consistently applied due to different base template parameters implementing or not the corresponding Gradient function

guitargeek approved these changes Jun 14, 2025

View reviewed changes

guitargeek merged commit 0d868ef into root-project:master Jun 14, 2025
23 of 24 checks passed

guitargeek mentioned this pull request Jun 14, 2025

Add missing override keywords wlav/CPyCppyy#54

Open

ferdymercury deleted the patch-13 branch June 15, 2025 01:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[ci] use override keyword and build with Wsuggestoverride #18918

[ci] use override keyword and build with Wsuggestoverride #18918

Uh oh!

ferdymercury commented May 30, 2025 •

edited

Loading

Uh oh!

github-actions bot commented May 30, 2025 •

edited

Loading

Uh oh!

ferdymercury commented Jun 2, 2025 •

edited

Loading

Uh oh!

pcanal left a comment

Uh oh!

ferdymercury commented Jun 13, 2025

Uh oh!

pcanal commented Jun 13, 2025

Uh oh!

pcanal commented Jun 13, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

guitargeek left a comment

Uh oh!

Uh oh!

Uh oh!

[ci] use override keyword and build with Wsuggestoverride #18918

[ci] use override keyword and build with Wsuggestoverride #18918

Uh oh!

Conversation

ferdymercury commented May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

This Pull request:

Changes or fixes:

Uh oh!

github-actions bot commented May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

ferdymercury commented Jun 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pcanal left a comment

Choose a reason for hiding this comment

Uh oh!

ferdymercury commented Jun 13, 2025

Uh oh!

pcanal commented Jun 13, 2025

Uh oh!

pcanal commented Jun 13, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

guitargeek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ferdymercury commented May 30, 2025 •

edited

Loading

github-actions bot commented May 30, 2025 •

edited

Loading

ferdymercury commented Jun 2, 2025 •

edited

Loading