[RF] Check that no two distinct input pdfs have same name in RooAddPdf and general RooAddPdf modernization#8004
Conversation
|
Starting build on |
|
@phsft-bot build |
|
Starting build on |
roofit/roofitcore/inc/RooAddPdf.h
Outdated
| virtual Bool_t forceAnalyticalInt(const RooAbsArg& /*dep*/) const { | ||
| // Force RooRealIntegral to offer all observables for internal integration |
There was a problem hiding this comment.
When you find this kind of old-style docs, move them above the function and replace // --> /// for doxygen to find it.
roofit/roofitcore/src/RooAddPdf.cxx
Outdated
| RooArgList partinCoefList ; | ||
|
|
||
| Bool_t first(kTRUE) ; | ||
| Bool_t first(true) ; |
There was a problem hiding this comment.
If you go towards standard c++ using kTRUE --> true, I would also use Bool_t --> bool.
| _recursive(kFALSE) | ||
| RooAddPdf(name,title) | ||
| { | ||
| _allExtendable = true; |
There was a problem hiding this comment.
Consider moving this in member initialiser list?
There was a problem hiding this comment.
This would give a warning: mem-initializer for ‘RooAddPdf::_allExtendable’ follows constructor delegation.
There was a problem hiding this comment.
Ah, I missed that it's a delegating constructor! 👍
* migrate Bool_t to bool * migrate Double_t to double outside of function signatures and member variables * replace `RooAbsCollection::getSize()` with STL-compatible `size()` and `empty()` * replace `TString` with `std::string` * update documentation of `RooAddPdf::forceAnalyticalInt` to doxygen format
|
Starting build on |
DeepCode's analysis on #3c2f25 found:
Top issues
👉 View analysis in DeepCode’s Dashboard | Configure the bot👉 The DeepCode service and API will be deprecated in August, 2021. Here is the information how to migrate. Thank you for using DeepCode 🙏 ❤️ !If you are using our plugins, you might be interested in their successors: Snyk's JetBrains plugin and Snyk's VS Code plugin. |
|
Build failed on ROOT-ubuntu16/nortcxxmod. Failing tests: |
lmoneta
left a comment
lmoneta
left a comment
There was a problem hiding this comment.
Looks good to me !
I have only the comments about getParameters/getObservables, which could if you wish be changed later in a different PR if you prefer
|
|
||
| // Retrieve the combined set of dependents of this PDF ; | ||
| RooArgSet *fullDepList = getObservables(nset) ; | ||
| auto fullDepList = std::unique_ptr<RooArgSet>{getObservables(nset)} ; |
There was a problem hiding this comment.
Hi,
Since you are moving to use unique pointer, would not be better to use instead the new functions which do not build the set, but they fill it::
RooArgSet fullDepList;
getObservables(nset, fullDepList);
There was a problem hiding this comment.
Good idea! I'll make these changes in a separate PR that uses the new functions also in more new places than RooAddPdf.
|
Build failed on ROOT-debian10-i386/cxx14. Errors:
|
4 participants
This addresses #8000.
The actual check is introduced in the 4th commit, all other commits contain general RooAddPdf code modernization.