Skip to content

refactor: remove TH1::AddDirectory from DQMGenericClient #49575

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.

Sign up for GitHub

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

Merged
cmsbuild merged 4 commits into from
Dec 15, 2025
Merged

refactor: remove TH1::AddDirectory from DQMGenericClient #49575

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9eef0a9
refactor: remove TH1::AddDirectory from DQMGenericClient
gabrielmscampos Dec 9, 2025
d282ce0
fix: clone TH1 from TObject to avoid having it deleted when the TObje…
gabrielmscampos Dec 9, 2025
27db91b
style: add '_' suffix to data members to follow CMS convetion
gabrielmscampos Dec 10, 2025
17086f1
refactor: use smart pointer instead of raw pointer
gabrielmscampos Dec 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 16 additions & 8 deletions DQMServices/Components/plugins/DQMGenericClient.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,18 +158,16 @@ class FitSlicesYTool {
public:
typedef dqm::harvesting::MonitorElement MonitorElement;
FitSlicesYTool(MonitorElement* me) {
const bool oldAddDir = TH1::AddDirectoryStatus();
TH1::AddDirectory(true);
// ... create your hists
TObjArray aSlices;
TH2F* h = me->getTH2F();
TF1 fgaus("fgaus", "gaus", h->GetYaxis()->GetXmin(), h->GetYaxis()->GetXmax(), TF1::EAddToList::kNo);
h->FitSlicesY(&fgaus, 0, -1, 0, "QNR SERIAL");
h->FitSlicesY(&fgaus, 0, -1, 0, "QNR SERIAL", &aSlices);
string name(h->GetName());
h0 = (TH1*)gDirectory->Get((name + "_0").c_str());
h1 = (TH1*)gDirectory->Get((name + "_1").c_str());
h2 = (TH1*)gDirectory->Get((name + "_2").c_str());
h3 = (TH1*)gDirectory->Get((name + "_chi2").c_str());
TH1::AddDirectory(oldAddDir);
h0 = fetchSlice(aSlices, name + "_0");
h1 = fetchSlice(aSlices, name + "_1");
h2 = fetchSlice(aSlices, name + "_2");
h3 = fetchSlice(aSlices, name + "_chi2");
}

/// Destructor
Expand Down Expand Up @@ -243,6 +241,16 @@ class FitSlicesYTool {
TH1* h1;
TH1* h2;
TH1* h3;

TH1* fetchSlice(TObjArray& arr, const std::string& name) {
Copy link
Contributor

@Dr15Jones Dr15Jones Dec 9, 2025

Choose a reason for hiding this comment

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

This seems to leak memory. Might I suggest

Suggested change
TH1* fetchSlice(TObjArray& arr, const std::string& name) {
std::unique_ptr<TH1> fetchSlice(TObjArray& arr, const std::string& name) {

Copy link
Member Author

@gabrielmscampos gabrielmscampos Dec 9, 2025

Choose a reason for hiding this comment

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

Hi @Dr15Jones, yes, I can implement your suggestion. May I ask why the first can lead to memory leak? Since it returns a raw pointer, the caller is responsible for deleting it (that is where I see the memory leak possibility), and in this case that deletion happens in the class destructor.

Copy link
Contributor

@Dr15Jones Dr15Jones Dec 9, 2025

Choose a reason for hiding this comment

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

I had not looked at the scope for the variables being used. As CMS convention is to use m_' prefix or a _ suffix for member data names I assumed these were local variables. So it sounds like it isn't a memory leak. Might I suggest changing those member variables to be std::unique_ptr<> anyway and having the function still return std::unique_ptr?

Copy link
Member Author

@gabrielmscampos gabrielmscampos Dec 10, 2025

Choose a reason for hiding this comment

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

Thanks. Done. I also took the opportunity to add the _ suffix to the member data names.

auto* obj = (TH1*)arr.FindObject(name.c_str());
if (!obj) {
throw cms::Exception("FitSlicesYTool") << "Cannot find slice histogram: " << name;
}
TH1* hnew = (TH1*)obj->Clone();
hnew->SetDirectory(nullptr);
return hnew;
}
};

typedef DQMGenericClient::MonitorElement ME;
Expand Down