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

support cstring in case #20130

Merged
merged 7 commits into from
Sep 1, 2022
Merged

support cstring in case #20130

merged 7 commits into from
Sep 1, 2022

Conversation

metagn
Copy link
Collaborator

@metagn metagn commented Aug 1, 2022

closes nim-lang/RFCs#350

Again a bit sloppy, but should work.

Can add experimental switch or whatever if desired.

@juancarlospaco
Copy link
Collaborator

Does it works when the cstring is nil ?.

@metagn
Copy link
Collaborator Author

metagn commented Aug 1, 2022

I thought it would be complicated to make it work but it's not

@Araq
Copy link
Member

Araq commented Aug 4, 2022

This is awesome! Since this will be in version 2.0 it doesn't need an experimental switch. It does need a changelog entry though and documentation updates.

@metagn
Copy link
Collaborator Author

metagn commented Aug 4, 2022

Done

@Varriount Varriount requested a review from Araq August 31, 2022 18:46
@Varriount Varriount added the Requires Araq To Merge PR should only be merged by Araq label Aug 31, 2022
@Araq Araq merged commit a95b639 into nim-lang:devel Sep 1, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Sep 1, 2022

Thanks for your hard work on this PR!
The lines below are statistics of the Nim compiler built from a95b639

Hint: mm: orc; threads: on; opt: speed; options: -d:release
163966 lines; 15.268s; 842.176MiB peakmem

capocasa pushed a commit to capocasa/Nim that referenced this pull request Mar 31, 2023
* implement case for cstring

for now just converts to string on C backend

* custom implementation for cstring

* remove leftover

* revert even more

* add nil + fix packages weird variant literal bug

* update docs
metagn added a commit to metagn/Nim that referenced this pull request Sep 27, 2024
Araq pushed a commit that referenced this pull request Sep 27, 2024
fixes #11422, refs #8336/#8333, refs #20130

The compiler generates conversion nodes *after* evaluating the branches
of case statements as constants, the reasoning is that case branches
accept constants of different types, like arrays or sets. But this means
that conversion nodes that need to be evaluated like converter calls
don't get evaluated as a constant for codegen. #8336 fixed this by
re-evaluating the node if an `nkHiddenCallConv` was created, and in
#20130 this logic also had to be added for `nkHiddenStdConv` for
cstrings. This logic was only for single case elements, it has now been
added to range elements as well to fix #11422. Additionally, all
conversion nodes are now evaluated for simplicity, but maybe this won't
pass CI.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Requires Araq To Merge PR should only be merged by Araq
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Accept cstrings for case selector in JS
4 participants