Skip to content

Conversation

@jakobnissen
Copy link
Member

Since SubString{String} has the same memory layout as String, this should be safe to do.

@jakobnissen
Copy link
Member Author

I believe the CI failures are unrelated

@Seelengrab
Copy link
Contributor

Seelengrab commented Nov 25, 2022

Would it be useful to add a @test_broken/@test_throws for the non-UnitRange cases? If Base ever allows such views on strings, this would no longer be safe but would go unnoticed without a "safeguard" test.

@jakobnissen
Copy link
Member Author

Not sure... I think SubString is used quite thoroughly in Base with implementations that take a pointer and assume that substrings are dense. So introducing non-dense substrings would break a lot of stuff.

@Seelengrab
Copy link
Contributor

That I don't know, I just thought it'd be good to safeguard this via a test that will explicitly break 🤷 If anything, it'll help a future implementor of non-UnitRange SubStrings to know that they have to be careful in other places too.

@fredrikekre
Copy link
Member

It is not possible to construct a SubString like that though, so not sure what test you are proposing.

@Seelengrab
Copy link
Contributor

As mentioned above, a @test_throws checking that trying to construct such a SubString still throws. E.g. something like

@test_throws MethodError s = @view str[1:2:end]

or something similar.

Since SubString{String} has the same memory layout as String, this should be
safe to do.
@fredrikekre fredrikekre merged commit b34fe1d into JuliaLang:master Nov 28, 2022
@fredrikekre
Copy link
Member

As mentioned above, a @test_throws checking that trying to construct such a SubString still throws.

That doesn't seem related to this PR at all, but perhaps you can add that in a separate PR if you think it is necessary.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants