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

change string indexing to avoid invalid indexes #3770

Merged
merged 6 commits into from
Aug 9, 2013

Conversation

JeffBezanson
Copy link
Member

I felt it was strange that using invalid indexes was fairly routine in our string code and API. Some functions returned them, and some functions practically expected them as input. There were i+1 and i-1s sprinkled here and there, to be fixed up eventually by a call to thisind. I don't think this is a good practice, and we should just use valid indexes. The only exception is the special index nextind(s, endof(s)), which identifies the empty space at the end of the string (for example when searching for the empty string).

The clearest example is in the commit message; search("∀ x", "∀", 1) gave 1:3. Using last(r) of that range could result in an invalid index error. Now it gives 1:1, a range of character indexes that is inclusive like all other index ranges. This can of course still be passed to getindex, which then no longer needs to call thisind.

Another way to look at this is that some functions tolerated invalid string indexes, and others gave errors, which is a bit confusing. It also made the behavior of isvalid, nextind, and prevind more ambiguous, since some indexes were completely invalid due to being out of bounds, while others were "less" invalid.

This also makes next and nextind more consistent: they give the same value when called on endof(s).

eliminate the thisind() function

For example, search("∀ x", "∀", 1) used to give 1:3, which doesn't
make much sense since the "3" is an artifact of the encoding and isn't
a valid index. Now it gives 1:1, or in general a:b where a is the index
of the first found character and b is the index of the last found character.
@Keno
Copy link
Member

Keno commented Jul 31, 2013

Is this in scope for 0.2?

@JeffBezanson
Copy link
Member Author

I guess we can vote. I think this is really important. @ViralBShah @StefanKarpinski

@Keno
Copy link
Member

Keno commented Jul 31, 2013

In that case, I vote in favor.

@JeffBezanson
Copy link
Member Author

bump

@StefanKarpinski
Copy link
Member

Yeah, I'm cool with this.

JeffBezanson added a commit that referenced this pull request Aug 9, 2013
change string indexing to avoid invalid indexes
@JeffBezanson JeffBezanson merged commit 2200bd3 into master Aug 9, 2013
@StefanKarpinski StefanKarpinski deleted the jb/strindexchanges branch November 16, 2013 16:43
IanButterworth pushed a commit that referenced this pull request Jan 24, 2024
…5539d (#53028)

Stdlib: Pkg
URL: https://github.com/JuliaLang/Pkg.jl.git
Stdlib branch: release-1.10
Julia branch: backports-release-1.10
Old commit: 11cf00df7
New commit: 70525539d
Julia version: 1.10.0
Pkg version: 1.10.0
Bump invoked by: @IanButterworth
Powered by:
[BumpStdlibs.jl](https://github.com/JuliaLang/BumpStdlibs.jl)

Diff:
JuliaLang/Pkg.jl@11cf00d...7052553

```
$ git log --oneline 11cf00df7..70525539d
70525539d Merge pull request #3770 from JuliaLang/backports-release-1.10
729ebe1e3 Avoid deleting existing artifacts when ignoring hashes. (#3768)
```

Co-authored-by: Dilum Aluthge <dilum@aluthge.com>
Drvi pushed a commit to RelationalAI/julia that referenced this pull request Jun 7, 2024
…5539d (JuliaLang#53028)

Stdlib: Pkg
URL: https://github.com/JuliaLang/Pkg.jl.git
Stdlib branch: release-1.10
Julia branch: backports-release-1.10
Old commit: 11cf00df7
New commit: 70525539d
Julia version: 1.10.0
Pkg version: 1.10.0
Bump invoked by: @IanButterworth
Powered by:
[BumpStdlibs.jl](https://github.com/JuliaLang/BumpStdlibs.jl)

Diff:
JuliaLang/Pkg.jl@11cf00d...7052553

```
$ git log --oneline 11cf00df7..70525539d
70525539d Merge pull request JuliaLang#3770 from JuliaLang/backports-release-1.10
729ebe1e3 Avoid deleting existing artifacts when ignoring hashes. (JuliaLang#3768)
```

Co-authored-by: Dilum Aluthge <dilum@aluthge.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking This change will break code needs decision A decision on this change is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants