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

implement String(::Base.Generator) #57072

Open
StefanKarpinski opened this issue Jan 16, 2025 · 7 comments · May be fixed by #57180
Open

implement String(::Base.Generator) #57072

StefanKarpinski opened this issue Jan 16, 2025 · 7 comments · May be fixed by #57180
Labels
good first issue Indicates a good issue for first-time contributors to Julia help wanted Indicates that a maintainer wants help on an issue or pull request strings "Strings!"

Comments

@StefanKarpinski
Copy link
Member

I was hoping that this would work:

String(Iterators.map(c->c+1, "Hello, world"))

Instead it's a method error. We should allow the String constructor to take a generator argument.

@StefanKarpinski StefanKarpinski added good first issue Indicates a good issue for first-time contributors to Julia help wanted Indicates that a maintainer wants help on an issue or pull request strings "Strings!" labels Jan 16, 2025
@ararslan
Copy link
Member

Perhaps a method that consumes general iterators would be worthwhile, e.g.

🍝(x, ::Union{HasLength,HasShape}) = String(copyto!(StringMemory(length(x)), x))
🍝(x, ::IsInfinite) = throw(MethodError(String, (x,)))
function 🍝(x, ::SizeUnknown)
    io = IOBuffer()
    for c in x
        print(io, convert(AbstractChar, c))
    end
    return String(take!(io))
end

String(itr) = 🍝(itr, IteratorSize(itr))

That would allow supporting things like String(Iterators.take("Hello, world", 5)) as well.

@KlausC
Copy link
Contributor

KlausC commented Jan 20, 2025

Attention: length is not correct in case of multi-byte characters.

@KlausC
Copy link
Contributor

KlausC commented Jan 20, 2025

Maybe Base.String(itr) = String(collect(itr)) would be sufficient (plus tests for all cases implied above)
EDIT: Not true, additionally must avoid stack overflow for e.g. String(19).

String(itr) = 🍝(itr, IteratorSize(itr))
🍝(x, ::IsInfinite) = throw(MethodError(String, (x,)))
🍝(x, ::IteratorSize) = String(collect(itr)::AbstractVector{<:AbstractChar})

@ararslan
Copy link
Member

Attention: length is not correct in case of multi-byte characters.

The length call in my suggested implementation is the length of the iterator with known length, not the length of a string. It's just used for allocating the memory required, and is actually identical to the implementation of String(::Memory) (IIRC).

@KlausC
Copy link
Contributor

KlausC commented Jan 21, 2025

@ararslan you may want to look at the following output to see what I mean:

julia> String( c for c in "ÄÖao")
"\xc4\xd6ao"

@ararslan
Copy link
Member

Ahhh yeah, thanks for the example, I see now. IteratorSize is probably not so useful a query then since HasLength only tells you that length can be assumed to be implemented. Something like

String(itr) = String([convert(AbstractChar, c) for c in itr])

should ensure you don't get a stack overflow on String(19), though the error you do get is rather opaque (MethodError on String(::Array{Char,0})).

@Priynsh
Copy link
Contributor

Priynsh commented Jan 28, 2025

String(x) = String_iterator(x, IteratorSize(x))
String_iterator(x, ::IsInfinite) = throw(MethodError(String, (x,)))
String_iterator(x, ::IteratorSize) = begin
    collected = collect(x)
    if !(isa(collected, AbstractVector) && all(x -> isa(x, Char), collected))
        throw(MethodError(String, (x,)))
    end
    return String(collected::AbstractVector{<:AbstractChar})
end

could something like this work? shows method error on String(19), works for all other cases provided :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Indicates a good issue for first-time contributors to Julia help wanted Indicates that a maintainer wants help on an issue or pull request strings "Strings!"
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants