-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[RF][RelNotes] Mention removal of const char*
to RooCmdArg conversion
#9833
[RF][RelNotes] Mention removal of const char*
to RooCmdArg conversion
#9833
Conversation
Starting build on |
ca3e1db
to
cc10163
Compare
Starting build on |
Build failed on ROOT-performance-centos8-multicore/default. Failing tests: |
Build failed on windows10/cxx14. Errors:
And 116 more |
Build failed on mac11/cxx17. Failing tests:
|
README/ReleaseNotes/v626/index.md
Outdated
### Compile-time protection against creating empty `RooCmdArg`s from strings | ||
|
||
The implicit [RooCmdArg](https://root.cern.ch/doc/v626/classRooCmdArg.html) constructor from `const char*` was removed to avoid the accidental construction of meaningless RooCmdArgs that only have a name but no payload. | ||
This causes new compiler errors in your code if you pass a string instead of a RooCmdArg to various RooFit functions, such as [RooAbsPdf::fitTo()](https://root.cern.ch/doc/v626/classRooAbsPdf.html#a5f79f16f4a26a19c9e66fb5c080f59c5). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here and everywhere else, please update to use the preferred root.cern
:
This causes new compiler errors in your code if you pass a string instead of a RooCmdArg to various RooFit functions, such as [RooAbsPdf::fitTo()](https://root.cern.ch/doc/v626/classRooAbsPdf.html#a5f79f16f4a26a19c9e66fb5c080f59c5). | |
This causes new compiler errors in your code if you pass a string instead of a RooCmdArg to various RooFit functions, such as [RooAbsPdf::fitTo()](https://root.cern/doc/v626/classRooAbsPdf.html#a5f79f16f4a26a19c9e66fb5c080f59c5). |
README/ReleaseNotes/v626/index.md
Outdated
```C++ | ||
pdf.fitTo(*data, "r"); | ||
// Will not compile anymore, as `"r"` is not a recognized command and will be ignored! | ||
// Instead use: gauss.fitTo(*data, RooFit::Range("r")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why gauss
instead of pdf
?
README/ReleaseNotes/v626/index.md
Outdated
```C++ | ||
pdf.fitTo(*data, "r"); | ||
// Will not compile anymore, as `"r"` is not a recognized command and will be ignored! | ||
// Instead use: gauss.fitTo(*data, RooFit::Range("r")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Instead use: gauss.fitTo(*data, RooFit::Range("r")); | |
// Instead, to restrict to a range called "r", use: | |
pdf.fitTo(*data, RooFit::Range("r")); |
README/ReleaseNotes/v626/index.md
Outdated
**Example** of an error that is now caught at compile time: confusing the [RooAbsPdf::fitTo()]() function signature with the one of [TH1::Fit()](https://root.cern.ch/doc/v626/classTH1.html#a63eb028df86bc86c8e20c989eb23fb2a) and passing the fit range name as a string literal: | ||
|
||
```C++ | ||
pdf.fitTo(*data, "r"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pdf.fitTo(*data, "r"); | |
pdf.fitTo(*data, "r"); // ERROR! |
or similar, to make it very visible that is is wrong :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Thanks Jonas for the update
RooFit got more strict and forbids the implicit construction of meaningless RooCmdArgs from strings. Our users should be warned about possible new compiler errors if they make this mistake.
cc10163
to
a578f35
Compare
Starting build on |
Thanks @Axel-Naumann and @lmoneta! I have updated the PR, I'll merge it without tests as it only affects the release notes. |
Build failed on ROOT-ubuntu16/nortcxxmod. Errors:
|
Build failed on windows10/cxx14. Errors:
|
Build failed on ROOT-performance-centos8-multicore/default. Errors:
|
Build failed on mac11/cxx17. Errors:
|
Build failed on mac1015/python3. Errors:
|
Build failed on ROOT-debian10-i386/cxx14. Errors:
|
Build failed on ROOT-ubuntu2004/soversion. Errors:
|
Starting build on |
Build failed on ROOT-performance-centos8-multicore/default. Errors:
|
Build failed on ROOT-debian10-i386/cxx14. Errors:
|
Build failed on mac11/cxx17. Errors:
|
Build failed on ROOT-ubuntu16/nortcxxmod. Errors:
|
Build failed on ROOT-ubuntu2004/soversion. Errors:
|
Build failed on windows10/cxx14. Errors:
|
Build failed on mac1015/python3. Errors:
|
RooFit got more strict and forbids the implicit construction of
meaningless RooCmdArgs from strings. Our users should be warned about
possible new compiler errors if they make this mistake.
Needs to be backported to the 6.26 branch (CMS already encountered such compiler errors in CMSSW).
This is a followup to #9747.