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

Fix memory leaks found by static analysis in leparse.cpp #1593

Merged
merged 1 commit into from
Sep 29, 2022

Conversation

lgritz
Copy link
Collaborator

@lgritz lgritz commented Sep 29, 2022

Several places in leparse.cpp were found by static analysis to have memory leaks -- results of calling _parse() that, in the case of early exists in error conditions, never got deleted.

I also did some other modernization: changed NULL to nullptr, changed some loops to more compact range-for, utilize some Strutil helpers that will be more efficient than the alternative std::string manipulation.

Signed-off-by: Larry Gritz lg@larrygritz.com

Several places in leparse.cpp were found by static analysis to have
memory leaks -- results of calling _parse() that, in the case of
early exists in error conditions, never got deleted.

I also did some other modernization: changed NULL to nullptr, changed
some loops to more compact range-for, utilize some Strutil helpers
that will be more efficient than the alternative std::string
manipulation.

Signed-off-by: Larry Gritz <lg@larrygritz.com>
Copy link
Contributor

@aconty aconty left a comment

Choose a reason for hiding this comment

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

Back when I wrote this there weren't range loops, so very good case of bit rot. The leaks are obvious enough that I should buy you a beer or lose my driving license :) Looking at this code makes me wanna rewrite it all with less new/delete.
Thanks!

@lgritz
Copy link
Collaborator Author

lgritz commented Sep 29, 2022

If I was smart, I would just have fixed the leak and moved on. (Yay for static analysis!)

I couldn't control myself on the range for loops, they were just sitting there calling to me.

I started to do a bigger refactor with std::unique_ptr and eliminating all the delete calls entirely, but stopped myself. There is zero need to go down that rathole, the code works as it is and the fact that it has barely been touched for 10 years attests to that.

@lgritz lgritz merged commit aea281d into AcademySoftwareFoundation:main Sep 29, 2022
@lgritz lgritz deleted the lg-lpexpleak branch September 29, 2022 17:07
lgritz added a commit to lgritz/OpenShadingLanguage that referenced this pull request Oct 10, 2022
…wareFoundation#1593)

Several places in leparse.cpp were found by static analysis to have
memory leaks -- results of calling _parse() that, in the case of
early exists in error conditions, never got deleted.

I also did some other modernization: changed NULL to nullptr, changed
some loops to more compact range-for, utilize some Strutil helpers
that will be more efficient than the alternative std::string
manipulation.

Signed-off-by: Larry Gritz <lg@larrygritz.com>
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.

None yet

2 participants