Skip to content

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

Merged
guitargeek merged 13 commits intoroot-project:masterfrom
ferdymercury:patch-13
Jun 14, 2025
Merged

[ci] use override keyword and build with Wsuggestoverride#18918
guitargeek merged 13 commits intoroot-project:masterfrom
ferdymercury:patch-13

Conversation

@ferdymercury
Copy link
Copy Markdown
Collaborator

@ferdymercury ferdymercury commented May 30, 2025 •
edited
Loading

This Pull request:

Changes or fixes:

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

fyi @wlav

@ferdymercury ferdymercury requested a review from dpiparo as a code owner May 30, 2025 07:43
@ferdymercury ferdymercury mentioned this pull request May 30, 2025
Closed
@ferdymercury ferdymercury changed the title [ci] Test issue 8302 [ci] Build debian with Wsuggestoverride, Test issue 8302 May 30, 2025
@ferdymercury ferdymercury requested a review from pcanal as a code owner May 30, 2025 08:03
@ferdymercury ferdymercury requested a review from guitargeek May 30, 2025 11:41
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 30, 2025 •
edited
Loading

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.

@ferdymercury ferdymercury requested a review from aaronj0 May 30, 2025 13:45
@ferdymercury ferdymercury requested a review from vepadulano as a code owner May 30, 2025 13:52
@ferdymercury ferdymercury requested a review from lmoneta as a code owner May 31, 2025 12:21
@ferdymercury ferdymercury marked this pull request as draft May 31, 2025 12:25
@ferdymercury ferdymercury changed the title [ci] Build debian with Wsuggestoverride, Test issue 8302 [ci] Build with Wsuggestoverride, Test issue 8302 May 31, 2025
@ferdymercury
[ci] Test issue 8302
926cf03 
[ci] move to alma9 as warnings
@ferdymercury
[core] missing final keywords
cc28be0 
[core] missing override in classdef

[test] add proper override in rqobjecttester
@ferdymercury
[pyroot] missing override keywords
5f38b64
@ferdymercury ferdymercury force-pushed the patch-13 branch 8 times, most recently from fd42072 to 7833f3b Compare June 2, 2025 08:08
@ferdymercury ferdymercury changed the title [ci] Build with Wsuggestoverride, Test issue 8302 [ci] use override keyword and build with Wsuggestoverride Jun 2, 2025
@ferdymercury ferdymercury added clean build Ask CI to do non-incremental build on PR and removed clean build Ask CI to do non-incremental build on PR labels Jun 2, 2025
@ferdymercury ferdymercury added the clean build Ask CI to do non-incremental build on PR label Jun 2, 2025
@ferdymercury ferdymercury reopened this Jun 2, 2025
@ferdymercury
Copy link
Copy Markdown
Collaborator Author

ferdymercury commented Jun 2, 2025 •
edited
Loading

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.

@ferdymercury ferdymercury marked this pull request as ready for review June 2, 2025 18:11
@ferdymercury ferdymercury requested a review from silverweed June 2, 2025 21:21
@ferdymercury ferdymercury mentioned this pull request Jun 4, 2025
Closed
@ferdymercury ferdymercury requested a review from pcanal June 11, 2025 21:51
@ferdymercury @pcanal
[io][nfc] add explanation on rationale of IsTObject check
9525e31 
Co-authored-by: Philippe Canal <pcanal@fnal.gov>
pcanal
pcanal approved these changes Jun 13, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@pcanal pcanal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

@pcanal pcanal closed this Jun 13, 2025
@pcanal pcanal reopened this Jun 13, 2025
@ferdymercury
Copy link
Copy Markdown
Collaborator Author

ferdymercury commented Jun 13, 2025

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
Copy link
Copy Markdown
Member

pcanal commented Jun 13, 2025

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 pcanal self-requested a review June 13, 2025 18:15
@pcanal
Copy link
Copy Markdown
Member

pcanal commented Jun 13, 2025

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.

guitargeek
guitargeek reviewed Jun 14, 2025
View reviewed changes
Comment thread math/mathcore/inc/Fit/LogLikelihoodFCN.h Outdated
guitargeek
guitargeek reviewed Jun 14, 2025
View reviewed changes
Comment thread .github/workflows/root-ci.yml
ferdymercury
ferdymercury commented Jun 14, 2025
View reviewed changes
Comment thread math/mathcore/inc/Fit/LogLikelihoodFCN.h Outdated
ferdymercury
ferdymercury commented Jun 14, 2025
View reviewed changes
Comment thread math/mathcore/inc/Fit/LogLikelihoodFCN.h Outdated
ferdymercury
ferdymercury commented Jun 14, 2025
View reviewed changes
Comment thread math/mathcore/inc/Fit/PoissonLikelihoodFCN.h Outdated
ferdymercury
ferdymercury commented Jun 14, 2025
View reviewed changes
Comment thread math/mathcore/inc/Fit/PoissonLikelihoodFCN.h Outdated
@ferdymercury
[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
guitargeek approved these changes Jun 14, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@guitargeek guitargeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👍

@guitargeek guitargeek merged commit 0d868ef into root-project:master Jun 14, 2025
23 of 24 checks passed
@guitargeek guitargeek mentioned this pull request Jun 14, 2025
Merged
@ferdymercury ferdymercury deleted the patch-13 branch June 15, 2025 01:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@guitargeek guitargeek guitargeek approved these changes

@dpiparo dpiparo Awaiting requested review from dpiparo dpiparo is a code owner

@aaronj0 aaronj0 Awaiting requested review from aaronj0

@vepadulano vepadulano Awaiting requested review from vepadulano vepadulano is a code owner

@lmoneta lmoneta Awaiting requested review from lmoneta lmoneta is a code owner

@jblomer jblomer Awaiting requested review from jblomer jblomer is a code owner

@martamaja10 martamaja10 Awaiting requested review from martamaja10 martamaja10 is a code owner

@silverweed silverweed Awaiting requested review from silverweed

@pcanal pcanal Awaiting requested review from pcanal pcanal is a code owner

Assignees

@dpiparo dpiparo

Labels

clean build Ask CI to do non-incremental build on PR

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ferdymercury @pcanal @guitargeek @dpiparo