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

disallow string range as of label #1147

Merged
merged 3 commits into from
Jan 30, 2024
Merged

disallow string range as of label #1147

merged 3 commits into from
Jan 30, 2024

Conversation

zerbina
Copy link
Collaborator

@zerbina zerbina commented Jan 30, 2024 •
edited by github-actions bot
Loading

Summary

Using ranges of strings (e.g., "a".."c" ) in of branches wasn't
rejected, but since both the C and JavaScript backends expect them to
not exist, it's likely that string ranges being allowed is
unintentional. An error is now reported for string ranges used as of
labels.

Details

  • the rsemStringRangeNotAllowed report is added
  • the callers of semBranchRange doesn't implement error propagation
    yet, so localReport is used

Tests

  • the tcasestmt test is changed to work with all targets
  • the float slice-list test from vm/tcasestmt is merged into the
    general tcasestmt test

@zerbina
sem: report an error for string ranges
481b655 
They were never explicitly supported, and using them resulted in
undefined behaviour.

Due to the lack of proper `nkError` propagation, the error has to be
reported with `localReport` in `semBranchRange` for now.
@zerbina
tests: enable tcasestmt for all targets
4a48297 
* test cases not depending on compile-time execution are moved out of
  `static` blocks
* `write` is replaced with `echo`. The `output` specification i
  updated accordingly
@zerbina
tests: merge vm/tcasestmt into the general tcasestmt
ff508d2 
Only the tests for float slice-lists are moved, the now-illegal string
slice-list tests are dropped.
@zerbina zerbina added test Add or improve tests compiler/sem Related to semantic-analysis system of the compiler language-design Language design syntax, semantics, types, statics and dynamics. labels Jan 30, 2024
@zerbina zerbina requested a review from saem January 30, 2024 17:55
@saem
Copy link
Collaborator

saem commented Jan 30, 2024

Fixed a minor typo in the PR body, reviewing the change.

saem
saem approved these changes Jan 30, 2024
View reviewed changes
Copy link
Collaborator

@saem saem left a comment

Choose a reason for hiding this comment

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

Error message looks good and the combined tests with extra coverage is much better.

@saem
Copy link
Collaborator

saem commented Jan 30, 2024

/merge

@github-actions GitHub Actions
Copy link

Merge requested by: @saem

Contents after the first section break of the PR description has been removed and preserved below:

Notes for Reviewers

This is a preparation for lowering case string statements into an compare + jump chain, which is itself a preparation for removing "simple" string constants (cnstString) from the VM. The goal is to simplify code generation and speeding up the loading of string literals.

@chore-runner chore-runner bot enabled auto-merge January 30, 2024 18:00
@chore-runner chore-runner bot added this pull request to the merge queue Jan 30, 2024
Merged via the queue into nim-works:devel with commit a4e0de6 Jan 30, 2024
19 checks passed
@zerbina zerbina mentioned this pull request Jan 30, 2024
Merged
@zerbina zerbina deleted the sem-disallow-string-range branch February 3, 2024 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@saem saem saem approved these changes

Assignees
No one assigned
Labels
compiler/sem Related to semantic-analysis system of the compiler language-design Language design syntax, semantics, types, statics and dynamics. test Add or improve tests
Projects
None yet
Milestone
Semantic analysis phase refactoring
Development

Successfully merging this pull request may close these issues.
2 participants
@zerbina @saem