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

disallow typedesc in arrays & move existing checks to types.typeAllowedAux #13261

Merged
merged 2 commits into from
May 29, 2020
Merged

Conversation

nc-x
Copy link
Contributor

@nc-x nc-x commented Jan 26, 2020

Closes #9932

@Araq
Copy link
Member

Araq commented Jan 26, 2020

But this is arbitrary, instead it should call types.typeAllowed or types.typeAllowed should be patched to disallow it.

@nc-x nc-x changed the title disallow typedesc in arrays disallow typedesc in arrays & move existing checks to types.typeAllowedAux Jan 28, 2020
compiler/sem.nim Outdated Show resolved Hide resolved
compiler/sem.nim Outdated Show resolved Hide resolved
compiler/sem.nim Outdated Show resolved Hide resolved
compiler/sem.nim Outdated Show resolved Hide resolved
result = t
else:
# XXX: This is still a horrible idea...
result = nil
Copy link
Member

Choose a reason for hiding this comment

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

can it still happen now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was existing code, so i don't know anything about this.
While reviewing, use the settings icon to Hide Whitespace Changes in order to see which code this PR changes.

compiler/types.nim Outdated Show resolved Hide resolved
@nc-x
Copy link
Contributor Author

nc-x commented May 28, 2020

All tests pass. Ready for review. @Araq

Edit: Why does [skip ci] not work?

@timotheecour
Copy link
Member

Edit: Why does [skip ci] not work?

you can upvote the issue I filed: microsoft/azure-pipelines-agent#2944
the workaround is to implement something like #13556 allow [nimDocOnly] in commit msg althouhg there are other ways to implement it

@Araq
Copy link
Member

Araq commented May 29, 2020

Do not use [skip ci], it's better to wait for the CIs, they are getting faster over time.

@nc-x
Copy link
Contributor Author

nc-x commented May 29, 2020

AFAICT, the currently used CI services azure, github actions as well as builds.sr.ht do not support [ci skip], so we cannot use it anyways.

@timotheecour
Copy link
Member

timotheecour commented May 29, 2020

Do not use [skip ci], it's better to wait for the CIs, they are getting faster over time.

it's still useful for in-progress work, but yes when it's getting finalized CI is almost always needed.
Another typical example (while still in development)is: your CI is almost finished completed, but a push (say to fix a typo) would restart all jobs. sad.

@Araq
Copy link
Member

Araq commented May 29, 2020

Superb work, many thanks!

@Araq Araq merged commit 4c08e64 into nim-lang:devel May 29, 2020
@nc-x nc-x deleted the 9932 branch May 29, 2020 10:02
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.

Error: internal error: expr(skType); unknown symbol
3 participants