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

Consistently initialize Exception.name across back-ends #292

Merged
merged 1 commit into from
May 1, 2022

Conversation

zerbina
Copy link
Collaborator

@zerbina zerbina commented Apr 30, 2022

Creating an Exception without raising it left it's name field as nil
on the JS and VM back-ends. For the C targets, the name initialization
was injected during code-gen.

Initializing the name field now either happens in newException or
when raising the exception and thus works the same across all back-ends.
Creating an exception without newException doesn't set the name
field on the C back-ends anymore.

In addition, when raising an exception on the JS target, the exception's
name is not overridden if it's non-nil.

This is a breaking change.


If the exception's name should also be automatically set when not using newException, I'd propose doing this in transf instead of during code-gen

@saem
Copy link
Collaborator

saem commented Apr 30, 2022

This feels more consistent, if some platform compatibility bits come up we can consider those as they arise and revisit.

Bors r+

bors bot added a commit that referenced this pull request Apr 30, 2022
292: Consistently initialize `Exception.name` across back-ends r=saem a=zerbina

Creating an `Exception` without raising it left it's `name` field as nil
on the JS and VM back-ends. For the C targets, the name initialization
was injected during code-gen.

Initializing the `name` field now either happens in `newException` or
when raising the exception and thus works the same across all back-ends.
Creating an exception without `newException` doesn't set the `name`
field on the C back-ends anymore.

In addition, when raising an exception on the JS target, the exception's
name is not overridden if it's non-nil.

This is a breaking change.



Co-authored-by: zerbina <[email protected]>
@bors
Copy link
Contributor

bors bot commented Apr 30, 2022

Build failed:

@zerbina
Copy link
Collaborator Author

zerbina commented Apr 30, 2022

The failing cpp test is due to the recent sem change

@saem
Copy link
Collaborator

saem commented Apr 30, 2022

The failing cpp test is due to the recent sem change

Exclude CPP from the test and leave a comment in the test with a comment (# knownIssue: ...).

@zerbina
Copy link
Collaborator Author

zerbina commented May 1, 2022

The failing cpp test should be fixed by #294. I'll rebase, once it's merged

@saem
Copy link
Collaborator

saem commented May 1, 2022

Merge in progress. 🎉

@saem
Copy link
Collaborator

saem commented May 1, 2022

bors r+

Creating an `Exception` without raising it left it's `name` field as nil
on the JS and VM back-ends. For the C targets, the name initialization
was injected during code-gen.

Initializing the `name` field now either happens in `newException` or
when raising the exception and thus works the same across all back-ends.
Creating an exception without `newException` doesn't set the `name`
field on the C back-ends anymore.

In addition, when raising an exception on the JS target, the exception's
name is not overriden if it's non-nil.

This is a breaking change.
@bors
Copy link
Contributor

bors bot commented May 1, 2022

Build succeeded:

@bors bors bot merged commit 19876f0 into nim-works:devel May 1, 2022
@zerbina zerbina deleted the exception-names branch May 3, 2022 16:36
@haxscramper haxscramper added this to the Sem phase refactoring milestone Nov 21, 2022
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