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

Support ROOT 6.30 and C++20 #873

Merged
merged 3 commits into from
Nov 30, 2023
Merged

Conversation

guitargeek
Copy link
Contributor

@guitargeek guitargeek commented Nov 28, 2023

The necessary changes to make combine compatible with ROOT 6.30 (and even master) compiled with the C++20 standard.

More detail in the commit descriptions.

The destructor and assignment operators are not allowed to include
template specializations in C++20.

They were not needed anyway, because the class itself is templated.

Also, generalize the assignment operators to take `TH1` and check for
the dimension at runtime. This is necessary to avoid other compiler
warnings, such as:

```
interface/FastTemplate.h:123:34: warning: ‘FastTemplate_t<T>& FastTemplate_t<T>::operator=(const TH1&) [with T = double]’ was hidden [-Woverloaded-virtual=]
  123 |         virtual FastTemplate_t & operator=(const TH1 &other) {
      |                                  ^~~~~~~~
interface/FastTemplate.h:284:24: note:   by ‘FastHisto2D_t<double>::operator=’
  284 |         FastHisto2D_t& operator=(const TH2 &other) {
```

That is a very valid warning! `FastHisto2D_t` inherits from
`FastTemplate_t`, and it's not good if their assignment operators take
different types.
@guitargeek
Copy link
Contributor Author

Hi @anigamova and @kcormi, would it be fine to merge this? It's a purely technical change that enables us to try out new ROOT versions in order to see how combine can benefit from the newest RooFit features

@kcormi
Copy link
Collaborator

kcormi commented Nov 30, 2023

Hi @guitargeek, thanks for these updates. Unfortunately with the relative paths, we had some issues previously as this was breaking the build in CMSSW, this popped up when we merged #809 and then was reverted to fix the compilation in #846. I think @nsmith- was planning to look into this, I wouldnt think it should be so difficult to fix, but I haven't done so since an original check when the build was breaking.

This will not work anymore in ROOT 6.30. The `double` value needs to be
retrieved from the proxy first.
@guitargeek
Copy link
Contributor Author

Ah ok, no problem! I have dropped the commit with the relative paths from this PR. After all, this was not relevant to support ROOT 6.30 with C++20, but only for my local build with CMake.

@kcormi
Copy link
Collaborator

kcormi commented Nov 30, 2023

Okay, great, thanks! I think its fine then, and builds fine in CMSSW as well.

@kcormi kcormi merged commit be488af into cms-analysis:main Nov 30, 2023
6 checks passed
@guitargeek guitargeek deleted the cpp20_fixes branch November 30, 2023 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants