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.

Sign up for GitHub

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

Jump to bottom

Fix #11833 and simplify injectdestructors #11926

Merged
merged 21 commits into from Aug 27, 2019
Merged

Fix #11833 and simplify injectdestructors #11926

merged 21 commits into from Aug 27, 2019

Conversation

Clyybber
Copy link
Contributor

No description provided.

@Araq Araq added the merge_when_passes_CI mergeable once green label Aug 11, 2019
@Clyybber Clyybber force-pushed the simplifyInjectdestructors branch 2 times, most recently from 8dcb308 to f0df203 Compare August 12, 2019 22:01
@Araq Araq removed the merge_when_passes_CI mergeable once green label Aug 14, 2019
Araq
Araq reviewed Aug 17, 2019
View reviewed changes
Copy link
Member

@Araq Araq left a comment

Choose a reason for hiding this comment

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

template recurse looks sloppy. The rest looks good.

@Clyybber
Copy link
Contributor Author

Not sure where the test failures on 32-bit come from...

@Araq Araq added this to the v1 milestone Aug 23, 2019
@Araq
Copy link
Member

Araq commented Aug 23, 2019

It fails with

Error: unhandled exception: injectdestructors.nim(474, 11) false nkChckRange64 [AssertionError]

Which are 32bit specific node kinds.

@Araq
Copy link
Member

Araq commented Aug 23, 2019

But to be honest, I don't see anymore how this rewrite is clearer than the current version. Sorry, but I would have to reject this, even if the tests were green.

@Araq Araq removed this from the v1 milestone Aug 23, 2019
@Clyybber
Copy link
Contributor Author

Clyybber commented Aug 23, 2019 •
edited

@Araq It is more explicit and includes the partially materialized arrays for proc arguments and discard. This explicitness also lead to me discovering a bug which is visible in the transformation of system.getEnv.
We also move string literals now.
This explicitness is an improvement over the current implementation IMO.
The amount of code duplication is also reduced with the new recurse proc, while at the same time being more explicit/"safe".
Sorry for the formatting, I dont have a physical keyboard at hand.

@Araq
Copy link
Member

Araq commented Aug 23, 2019

Ok, in the meantime, the bugfix is also here: #12018

@Clyybber
Correct fix
8f22d3d 
Which unfortunately breaks bootstrapping..
@Clyybber Clyybber closed this Aug 25, 2019
@Clyybber Clyybber reopened this Aug 25, 2019
@Clyybber
Fix bootstrapping
a2fdd2a
@Clyybber
90% pass
ab95772
@Clyybber
Work around stupid case expr
09ee9d1
@Clyybber
Handle empty nkDiscardStmt
8bea4a8
@Clyybber
Cleanup and fix some issues
7b908a1 
One improvement over #devel is visible in the transformation of getEnv. With this approach we move to result whenever possible.
@Clyybber
Comment out debug code
140951b
Araq
Araq reviewed Aug 26, 2019
View reviewed changes
tests/destructor/t11833.nim Outdated Show resolved Hide resolved
@Clyybber
Copy link
Contributor Author

Unrelated test failures..

@Clyybber
Copy link
Contributor Author

@Araq ping

@Araq Araq merged commit 20dec10 into nim-lang:devel Aug 27, 2019
@Araq
Copy link
Member

Araq commented Sep 11, 2019

Since this causes regressions, I'm reverting it. The statement/expression split is outdated, I don't use it anymore for transformations, see #12168 for how I would do it. It currently fails every test though...

Araq added a commit that referenced this pull request Sep 11, 2019
@Araq
Revert "Refactored injectdestructors.nim (#11926)"
a1a6ed4 
This reverts commit 20dec10.
@Araq Araq mentioned this pull request Sep 11, 2019
Merged
Araq added a commit that referenced this pull request Sep 11, 2019
@Araq
Revert "Refactored injectdestructors.nim (#11926)" (#12169)
33b8a00 
This reverts commit 20dec10.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Araq Araq Araq left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@Clyybber @Araq