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

refactor MonitorObject and adding Copy capabilities #2450

Merged
merged 3 commits into from
Nov 22, 2024

Conversation

justonedev1
Copy link
Collaborator

when working on QC-1201 and implementing Copy function/ctor/operator I found that Monitor Object is weirdly written, moreover there are possiblities, where the class can leak memory. So I refactored it a bit while adding Copy function, ctor and operator.

Copy link
Collaborator

@Barthelemy Barthelemy left a comment

Choose a reason for hiding this comment

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

Your hands must be dirty and dusty after touching such an antique piece of code !

I agree with all this.
Not sure if we should merge it now or wait post-HI.

@@ -141,6 +143,9 @@ class MonitorObject : public TObject
// tells Merger to create an object with data from the last cycle only on the side of the complete object
bool mCreateMovingWindow = false;

void releaseObject();
void cloneAndSetObject(const MonitorObject&);

ClassDefOverride(MonitorObject, 12);
Copy link
Collaborator

Choose a reason for hiding this comment

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

you must increase the class version otherwise ROOT can't do the schema evolution.
not super important now that we don't store MO in the QCDB but still...

@justonedev1
Copy link
Collaborator Author

the thing is that I need this PR for QC-1201. But I don't know how necessary that ticket is... probably not much for HI. So I am 100% fine to just wait after HI...

Copy link
Collaborator

@knopers8 knopers8 left a comment

Choose a reason for hiding this comment

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

As I was talking to you the other day, I am not very happy about having a unique_ptr which does not always own the pointer, but I don't see a better way to do this. And I support the intention of the changes. Thank you for the initiative!

@justonedev1
Copy link
Collaborator Author

@ktf Can you please take a look at the reason why the build checks are failing?

@justonedev1 justonedev1 merged commit 13084fd into AliceO2Group:master Nov 22, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants