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

push pragma no longes applies the raises pragmas to vars/lets/consts #10026

Merged
merged 1 commit into from Dec 30, 2018

Conversation

Projects
None yet
3 participants
@nc-x
Copy link
Contributor

nc-x commented Dec 17, 2018

Closes #10015

@nc-x nc-x force-pushed the nc-x:pragma-1 branch from de013b5 to 09c4908 Dec 17, 2018

@Araq

This comment has been minimized.

Copy link
Member

Araq commented Dec 17, 2018

But why does the push even apply to these symbol kinds to begin with?

@nc-x nc-x changed the title raises pragma is ignored for vars/lets/consts push pragma no longes applies pragmas to vars/lets/consts Dec 18, 2018

@nc-x

This comment has been minimized.

Copy link
Contributor

nc-x commented Dec 18, 2018

Done the required changes.

@Araq

This comment has been minimized.

Copy link
Member

Araq commented Dec 18, 2018

This is great but a subtle breaking change, I think {.push importc.} worked for vars before and now it doesn't. At least this needs a bold changelog entry, but we also need to think about a better migration path.

@timotheecour

This comment has been minimized.

Copy link
Collaborator

timotheecour commented Dec 19, 2018

can we be more selective instead ? so that raises(and maybe other TBD as we find more bugs) wouldn't apply to var/let/const but other pragmas would

@Araq

This comment has been minimized.

Copy link
Member

Araq commented Dec 19, 2018

can we be more selective instead ? so that raises(and maybe other TBD as we find more bugs) wouldn't apply to var/let/const but other pragmas would

Hmm yeah. Sounds much better for backwards compat.

@nc-x

This comment has been minimized.

Copy link
Contributor

nc-x commented Dec 19, 2018

So should I revert f632141 and let the original patch remain 09c4908

@Araq

This comment has been minimized.

Copy link
Member

Araq commented Dec 19, 2018

09c4908 is bad because it allows for an explicit var foo: int {.raises: [].} which makes no sense. We need an "comes from push" flag in the pragma handling.

@nc-x nc-x force-pushed the nc-x:pragma-1 branch 2 times, most recently from d6f8bab to be93cb4 Dec 27, 2018

@nc-x

This comment has been minimized.

Copy link
Contributor

nc-x commented Dec 27, 2018

@Araq done.

@nc-x nc-x changed the title push pragma no longes applies pragmas to vars/lets/consts push pragma no longes applies the raises pragmas to vars/lets/consts Dec 27, 2018

Show resolved Hide resolved compiler/ast.nim Outdated
Show resolved Hide resolved compiler/pragmas.nim Outdated

@nc-x nc-x force-pushed the nc-x:pragma-1 branch from be93cb4 to 7c2e69c Dec 28, 2018

@nc-x

This comment has been minimized.

Copy link
Contributor

nc-x commented Dec 28, 2018

cc @Araq

@Araq Araq merged commit 527b774 into nim-lang:devel Dec 30, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@nc-x nc-x deleted the nc-x:pragma-1 branch Dec 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment