Skip to content

expand the IteratorSize(::Type{Char}) method to cover AbstractChar #58524

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nsajko
Copy link
Contributor

@nsajko nsajko commented May 25, 2025

The narrow scope of the method seems like a mistake:

  • The immediately surrounding lines of code all use AbstractChar, not Char.

  • ndims already returns 0 for new AbstractChar subtypes, so it seems like it would be more consistent for IteratorSize to return HasShape{0}() instead of the default, Base.HasLength().

  • Broadcasting already treats AbstractChar as zero-dimensional, since PR fix char-broadcast PR to work with AbstractChar #41073.

Historic references:

  • PR Add IteratorSize for Char #29819 added the IteratorSize(::Type{Char}) method. Not sure why was the method restricted to Char instead of to all subtypes of AbstractChar.

@nsajko nsajko added strings "Strings!" design Design of APIs or of the language itself iteration Involves iteration or the iteration protocol labels May 25, 2025
The narrow scope of the method seems like a mistake:

* The immediately surrounding lines of code all use `AbstractChar`,
  not `Char`.

* `ndims` already returns `0` for new `AbstractChar` subtypes, so it
  seems like it would be more consistent for `IteratorSize` to return
  `HasShape{0}()` instead of the default, `Base.HasLength()`.

* Broadcasting already treats `AbstractChar` as zero-dimensional, since
  PR JuliaLang#41073.

Historic references:

* PR JuliaLang#29819 added the `IteratorSize(::Type{Char})` method. Not sure
  why was the method restricted to `Char` instead of to all subtypes
  of `AbstractChar`.
@nsajko nsajko force-pushed the AbstractChar_IteratorSize branch from e483925 to 349d133 Compare May 31, 2025 09:38
Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

Looks good to me

@nsajko nsajko added the merge me PR is reviewed. Merge when all tests are passing label May 31, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design Design of APIs or of the language itself iteration Involves iteration or the iteration protocol merge me PR is reviewed. Merge when all tests are passing strings "Strings!"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants