Skip to content

fix warnings#187

Merged
gr5 merged 1 commit intogithubdoe:masterfrom
atsju:JST/fixWarn
May 15, 2025
Merged

fix warnings#187
gr5 merged 1 commit intogithubdoe:masterfrom
atsju:JST/fixWarn

Conversation

@atsju
Copy link
Collaborator

@atsju atsju commented May 15, 2025

No description provided.

@atsju
Copy link
Collaborator Author

atsju commented May 15, 2025

This feels likepotential memory leek. not "yet another inoffensive warning"
image
Will look at it later

@gr5 gr5 merged commit b3ac6d8 into githubdoe:master May 15, 2025
8 checks passed
@gr5
Copy link
Collaborator

gr5 commented May 15, 2025

Julien, I looked a little more at the line of code above.

When class ProfilePlot constructor is called, the "new" creates a class instance called profilePlotPicker. And that's the only time profilePlotPicker is created.

ProfilePlot appears to be only ever created in the constructor of class MainWindow. And there is only one instance of that ever.

So there is only ever one instance of profilePlotPicker.

So I think one can just get rid of the variable and simply change it to:

new profilePlotPicker(m_plot);

Yes there's technically a memory leak. Each of these should destruct their members when they are destructed. But it's not really a problem.

@atsju atsju deleted the JST/fixWarn branch May 16, 2025 05:38
@atsju
Copy link
Collaborator Author

atsju commented May 16, 2025

Thanks for the look. I had same conclusion.

However I not here to remove warnings. I will try to actually get rid of the leak. And here again, not because the leak is an actual problem but because it masks other leaks in analysis tools that are actuel problems.

Might sound a bit philosophical an me being stubborn (which I am) but it actually helps fixing problems and maintaining the code in long term.

Anyway, thanks for review and tests :) I appreciate the work being done here.

@githubdoe
Copy link
Owner

I had intended but forgot to free profilePlotPicker in the parent destructor.

@gr5
Copy link
Collaborator

gr5 commented May 17, 2025

So Julien - do you want to fix this memory leak soon or should I proceed with the release tomorrow? Or should we leave it for next release?

I can actually fix the memory leak thing. I can do it for sure on Sunday. Possibly tomorrow. I know it's simple and quick but I was pretty crazy busy today and not sure how much time I'll have tomorrow. Probably plenty of time but not certain.

@atsju
Copy link
Collaborator Author

atsju commented May 17, 2025

So Julien - do you want to fix this memory leak soon or should I proceed with the release tomorrow? Or should we leave it for next release?

I can actually fix the memory leak thing. I can do it for sure on Sunday. Possibly tomorrow. I know it's simple and quick but I was pretty crazy busy today and not sure how much time I'll have tomorrow. Probably plenty of time but not certain.

Sorry, haven't done C++ and QT for 2 years. I had a look and if I understand correctly there is a parent so no memory leak ?
I have not been able to setup Valgrind to double check for the moment. Will try to fix the 2 remaining warnings over the weekend but if you want to release early, I see no problem for user.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants