Call finalisers for embedded fields when parent type has no finalizer. #1629
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Praetonus
force-pushed
the
gen-final-embed
branch
from
501b27f
to
522126d
Compare
jemc
requested changes
Mar 3, 2017
src/libponyc/reach/reach.c
Outdated
| { | ||
| if(ast_id(member) == TK_EMBED) | ||
| { | ||
| if(embed_has_finaliser(ast_type(member), str_final)) |
There was a problem hiding this comment.
Couldn't this be refactored to be a part of the above if statement, using &&?
Praetonus
force-pushed
the
gen-final-embed
branch
from
522126d
to
1501add
Compare
|
I've rebased on latest master to pick up the changes merged from #1621. |
jemc
reviewed
Mar 5, 2017
src/libponyc/reach/reach.c
Outdated
| embed_has_finaliser(ast_type(member), str_final)) | ||
| { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
I don't think the braces are needed here, and the indentation is a bit off.
dipinhora
added a commit
to dipinhora/ponyc
that referenced
this pull request
Mar 9, 2017
This commit changes how the Windows memory allocation/commitment is done by switching from committing all memory immediately to only committing memory dynamically as it gets allocated. This resolves the Windows errors that PR ponylang#1614 and PR ponylang#1629 have been encountering related to `ponyint_virt_alloc` and `The paging file is too small for this operation to complete.`
SeanTAllen
force-pushed
the
master
branch
6 times, most recently
from
97b1943
to
21d37aa
Compare
Praetonus
force-pushed
the
gen-final-embed
branch
from
1501add
to
470dc51
Compare
|
I've rebased on latest master after the merge of #1677. |
|
The new tests are causing a segfault on Linux and I can't reproduce it. I'll try to investigate. |
Praetonus
force-pushed
the
gen-final-embed
branch
from
7f88686
to
aa0cae2
Compare
This fixes a case overlooked in 087ebc7. In that commit, the finaliser of an embedded field was only called if the parent object (or actor) had an explicit finaliser. This change adds implicit finalisers to objects that must finalise an embedded field, including recursively.
Praetonus
force-pushed
the
gen-final-embed
branch
from
a1269d8
to
bb8aac4
Compare
|
The tests are passing. I've also made the changes you requested @jemc. |
jemc
changed the title
jemc
added
the
changelog - fixed
jemc
approved these changes
Mar 17, 2017
ponylang-main
added a commit
that referenced
this pull request
Mar 17, 2017
georgemarrows
added a commit
to georgemarrows/ponyc
that referenced
this pull request
Jun 28, 2017
Praetonus
pushed a commit
that referenced
this pull request
Jun 28, 2017
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes a case overlooked in 087ebc7. In that commit, the finaliser of an embedded field was only called if the parent object (or actor) had an explicit finaliser. This change adds implicit finalisers to objects that must finalise an embedded field, including recursively.
PS: If this gets merged before the pending release this shouldn't get a changelog entry since it is covered by the one from #1586.