Skip to content

[RF] Correctly override SetName in RooDataHist and RooDataSet#11417

Merged
guitargeek merged 1 commit intoroot-project:masterfrom
guitargeek:issue-11414
Sep 23, 2022
Merged

[RF] Correctly override SetName in RooDataHist and RooDataSet#11417
guitargeek merged 1 commit intoroot-project:masterfrom
guitargeek:issue-11414

Conversation

@guitargeek
Copy link
Copy Markdown
Contributor

@guitargeek guitargeek commented Sep 22, 2022

When overriding SetName in RooDataHist and RooDataSet, we need to use the function from the direct base class RooAbsData, because this one is already overriding the TNamed function to deal with the RooNameReg correctly.

Closes #11414.

@guitargeek
[RF] Correctly override SetName in RooDataHist and RooDataSet
e9866f9 
When overriding `SetName` in RooDataHist and RooDataSet, we need to use
the function from the direct base class RooAbsData, because this one is
already overriding the TNamed function to deal with the RooNameReg
correctly.

Closes root-project#11414.
@guitargeek guitargeek requested a review from lmoneta as a code owner September 22, 2022 21:43
@guitargeek guitargeek self-assigned this Sep 22, 2022
@phsft-bot
Copy link
Copy Markdown

phsft-bot commented Sep 22, 2022

Starting build on ROOT-debian10-i386/soversion, ROOT-performance-centos8-multicore/cxx17, ROOT-ubuntu18.04/nortcxxmod, ROOT-ubuntu2004/python3, mac1015/cxx17, mac11/cxx14, windows10/cxx14
How to customize builds

@phsft-bot
Copy link
Copy Markdown

phsft-bot commented Sep 23, 2022

Build failed on windows10/cxx14.
Running on null:C:\build\workspace\root-pullrequests-build
See console output.

@phsft-bot
Copy link
Copy Markdown

phsft-bot commented Sep 23, 2022

Build failed on mac1015/cxx17.
Running on macitois21.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

lmoneta
lmoneta reviewed Sep 23, 2022
View reviewed changes
Copy link
Copy Markdown
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.

Is it needed to have another name stored in RooAbsData ? Why we cannot use the name in TNamed ?

@guitargeek
Copy link
Copy Markdown
Contributor Author

guitargeek commented Sep 23, 2022

Is it needed to have another name stored in RooAbsData ? Why we cannot use the name in TNamed ?

There is no other name stored in RooAbsData. RooAbsData only overrides TNamed::SetData() to notify the RooNameReg correctly of the name change to get a new namePtr() for fast lookups. If we now skip this override by calling TNamed::SetName() directly, the name pointers are broken, causing the issue reported.

lmoneta
lmoneta approved these changes Sep 23, 2022
View reviewed changes
Copy link
Copy Markdown
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.

Thank you Jonas for the answer!
It then good to merge this !

@guitargeek guitargeek merged commit f27c7bc into root-project:master Sep 23, 2022
@guitargeek guitargeek deleted the issue-11414 branch September 23, 2022 09:12
This was referenced Sep 23, 2022
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lmoneta lmoneta lmoneta approved these changes

@hageboeck hageboeck Awaiting requested review from hageboeck

Assignees

@guitargeek guitargeek

Labels

bug in:RooFit

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[RF] Renaming dataset fails when total number of dataset in workspace reached 10

3 participants

@guitargeek @phsft-bot @lmoneta