Skip to content
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

[math] Don't #include "Math/Error.h" and "Math/Util.h" in Minuit2 headers #13646

Merged
merged 1 commit into from
Sep 14, 2023

Conversation

guitargeek
Copy link
Contributor

@guitargeek guitargeek commented Sep 13, 2023

The "Math/Error.h" header is not shipped with standalone Minuit2.

It is not a perfect solution to just ship it with Minuit2, because the Error.h header behaves differently depending on the MATHCORE_STANDALONE macro being defined or not. The code would only work correctly if the user defines the MATHCORE_STANDALONE herself in the user code that uses standalone Minuit2, which would be annoying.

Instead, this commit proposes another solution to the problem: for all headers also used in Minuit2 standalone, MathCore
moves the definitions of all functions that use Math/Error.h out of the header files in the cxx files. Like this, the Math/Error.h is only a build dependency of standalone Minuit2, and the user doesn't need to define the MATHCORE_STANDALONE macro for it to work.

Including the "Math/Util.h" header needs to be avoided for similar reasons (it's about another preprocessor macro related to veccore).

@phsft-bot
Copy link
Collaborator

Starting build on ROOT-performance-centos8-multicore/soversion, ROOT-ubuntu2204/nortcxxmod, ROOT-ubuntu2004/python3, mac11/noimt, mac12arm/cxx20, windows10/default
How to customize builds

@guitargeek guitargeek changed the title [math] No #include "Math/Error.h" in mathcore headers used by Minuit2 [math] Don't #include "Math/Error.h" and "Math/Utils.h" in mathcore headers used by Minuit2 Sep 13, 2023
@guitargeek guitargeek changed the title [math] Don't #include "Math/Error.h" and "Math/Utils.h" in mathcore headers used by Minuit2 [math] Don't #include "Math/Error.h" and "Math/Utils.h" in Minuit2 headers Sep 13, 2023
@phsft-bot
Copy link
Collaborator

Build failed on mac11/noimt.
Running on macphsft23.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Errors:

  • [2023-09-13T12:35:53.153Z] CMake Error at cmake/modules/SearchInstalledSoftware.cmake:1744 (message):
  • [2023-09-13T12:35:53.153Z] CMake Error at /Users/sftnight/build/workspace/root-pullrequests-build/rootspi/jenkins/root-build.cmake:1198 (message):

@guitargeek guitargeek changed the title [math] Don't #include "Math/Error.h" and "Math/Utils.h" in Minuit2 headers [math] Don't #include "Math/Error.h" and "Math/Util.h" in Minuit2 headers Sep 13, 2023
The "Math/Error.h" header is not shipped with standalone Minuit2.

It is not a perfect solution to just ship it with Minuit2, because the
Error.h header behaves differently depending on the
`MATHCORE_STANDALONE` macro being defined or not. The code would only
work correctly if the user defines the `MATHCORE_STANDALONE` herself in
the user code that uses standalone Minuit2, which would be annoying.

Instead, this commit proposes another solution to the problem: for all
headers also used in Minuit2 standalone, MathCore
moves the definitions of all functions that use `Math/Error.h` out of
the header files in the cxx files. Like this, the `Math/Error.h` is only
a build dependency of standalone Minuit2, and the user doesn't need to
define the `MATHCORE_STANDALONE` macro for it to work.

Including the "Math/Util.h" header needs to be avoided for similar
reasons (it's about another preprocessor macro related to `veccore`).
@phsft-bot
Copy link
Collaborator

Starting build on ROOT-performance-centos8-multicore/soversion, ROOT-ubuntu2204/nortcxxmod, ROOT-ubuntu2004/python3, mac11/noimt, mac12arm/cxx20, windows10/default
How to customize builds

@phsft-bot
Copy link
Collaborator

Build failed on mac11/noimt.
Running on macphsft23.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Errors:

  • [2023-09-13T13:05:30.930Z] CMake Error at cmake/modules/SearchInstalledSoftware.cmake:1744 (message):
  • [2023-09-13T13:05:30.930Z] CMake Error at /Users/sftnight/build/workspace/root-pullrequests-build/rootspi/jenkins/root-build.cmake:1198 (message):

@github-actions
Copy link

Test Results

       10 files         10 suites   2d 4h 3m 39s ⏱️
  2 492 tests   2 489 ✔️ 0 💤 3
23 851 runs  23 848 ✔️ 0 💤 3

For more details on these failures, see this check.

Results for commit 466258e.

Copy link
Member

@lmoneta lmoneta left a comment

Choose a reason for hiding this comment

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

LGTM!
Thank you for fixing this!

@guitargeek guitargeek merged commit 160d396 into root-project:master Sep 14, 2023
9 of 14 checks passed
@guitargeek guitargeek deleted the minuit2_math_error branch September 14, 2023 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants