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 leak in ASTvariable_declaration #1576

Merged
merged 2 commits into from
Sep 9, 2022

Conversation

lgritz
Copy link
Collaborator

@lgritz lgritz commented Sep 6, 2022

No description provided.

@lgritz lgritz force-pushed the lg-astleak branch 3 times, most recently from 4398e8c to 4b1114f Compare September 8, 2022 05:54
@lgritz
Copy link
Collaborator Author

lgritz commented Sep 8, 2022

This look ok?

Copy link
Contributor

@fpsunflower fpsunflower left a comment

Choose a reason for hiding this comment

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

LGTM

Though the fact that this node sometimes owns the memory and sometimes does not seems a bit confusing.

Would there be a way to rig this to use unique_ptr? Or is the pointer potentially valid but non-owning in some cases?

@lgritz
Copy link
Collaborator Author

lgritz commented Sep 8, 2022

If it puts it into the symbol table (which it does if it's not metadata), this pointer is non-owning and the symbol table itself is responsible for deleting it. But for metadata, this is the only pointer to it.

Let me take another look at this, maybe there is a more clear way to organize it to not be so confusing.

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

lgritz commented Sep 9, 2022

@fpsunflower I pushed an update. Do you think this makes the logic clearer?

Copy link
Contributor

@fpsunflower fpsunflower left a comment

Choose a reason for hiding this comment

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

I like it!

@lgritz lgritz merged commit 1a76706 into AcademySoftwareFoundation:main Sep 9, 2022
@lgritz lgritz deleted the lg-astleak branch September 9, 2022 04:04
lgritz added a commit to lgritz/OpenShadingLanguage that referenced this pull request Oct 6, 2022
…#1576)

* Fix memory leak in ASTvariable_declaration

Signed-off-by: Larry Gritz <lg@larrygritz.com>
chellmuth pushed a commit to chellmuth/OpenShadingLanguage that referenced this pull request Sep 6, 2024
* Bug fix: assigning init-op param its default value as instance got
  lost (AcademySoftwareFoundation#1578)

* Fix oslc memory leak in ASTvariable_declaration (AcademySoftwareFoundation#1576)

See merge request spi/dev/3rd-party/osl-feedstock!32
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.

2 participants