Skip to content

Conversation

beef331
Copy link
Collaborator

@beef331 beef331 commented Oct 10, 2022

No description provided.

@beef331
Copy link
Collaborator Author

beef331 commented Oct 10, 2022

Seems an issue will be converters to string do not fire for openarray[char]. Perhaps for converters string should work with openArray[char]?

@Araq
Copy link
Member

Araq commented Oct 10, 2022

Seems an issue will be converters to string do not fire for openarray[char]. Perhaps for converters string should work with openArray[char]?

Nah, it's fine this way IMO. The real issue is here that it's a breaking change which would be easily avoidable by introducing duplication in the form of a new std / parseslices module.

@beef331
Copy link
Collaborator Author

beef331 commented Oct 10, 2022

In the case where people want a 'weak distinct' it would give the ability to optimise a copy away as the following. Given openarray[char] cannot be used in a converter one cannot really do this.

type NotString = distinct string
converter toString*(s: NotString): lent string = string(s)

@Araq
Copy link
Member

Araq commented Oct 10, 2022

I understand the problem but introducing "converter chaining" at this point would be far too an invasive change. So the next best option is str / parseslices

@beef331 beef331 force-pushed the openarrayParseutils branch from bcbbbd2 to 399a967 Compare October 10, 2022 20:49
@beef331 beef331 changed the title Refactored parseutils to use openarray[char] instead of string Added 'parseslices' module Oct 10, 2022
@beef331 beef331 force-pushed the openarrayParseutils branch from 399a967 to 869eb81 Compare October 10, 2022 21:10
@Varriount
Copy link
Contributor

Varriount commented Oct 10, 2022

This is a kludgy hack. Is there any alternative to creating a copy of an entire module? Why can't the duplicate functions just be added to parseutils?

I understand the problem but introducing "converter chaining" at this point would be far too an invasive change.

Even if it's only for special cases (like openarrays)?

@beef331
Copy link
Collaborator Author

beef331 commented Oct 10, 2022

Why can't the duplicate functions just be added to parseutils

I forgot that openarray[char] and string did not cause a mismatch with converters, so I guess it could be added there, make all the base procedures operate on openarray[char] but have the string versions call the openarray variants.

@Araq
Copy link
Member

Araq commented Oct 11, 2022

Even if it's only for special cases (like openarrays)?

Especially if it's only for special cases. These come back and bite you. ;-)

@planetis-m
Copy link
Contributor

For custom string types it's nice to have implicit converters to openArray:

{.experimental: "views".}

converter toOpen(x: MyString): openArray[byte] =
  toOpenArray(x.data, 0, x.data.high)

although I don't know if this is the direction we are going, or is it going to cause problems?

@Varriount
Copy link
Contributor

although I don't know if this is the direction we are going, or is it going to cause problems?

The direction I'd like here is to solve this problem in a simple way, that minimizes redundancy. Ideally by just adding overloads and/or type constraints to procedures in parseutils.

@Araq
Copy link
Member

Araq commented Oct 11, 2022

Adding overloads that support openArray[char] seems to be the best option for now.

@beef331 beef331 force-pushed the openarrayParseutils branch from 869eb81 to 400681a Compare October 11, 2022 20:03
@beef331 beef331 changed the title Added 'parseslices' module Added 'openArray[char' overloads to 'std/parseutils' Oct 11, 2022
@beef331 beef331 marked this pull request as ready for review October 11, 2022 20:04
@beef331 beef331 force-pushed the openarrayParseutils branch 2 times, most recently from a6f31bf to e72cdcc Compare October 11, 2022 20:15
@beef331 beef331 force-pushed the openarrayParseutils branch from e72cdcc to ac883a4 Compare October 11, 2022 21:19
@beef331 beef331 changed the title Added 'openArray[char' overloads to 'std/parseutils' Added 'openArray[char]' overloads to 'std/parseutils' Oct 11, 2022
@beef331 beef331 force-pushed the openarrayParseutils branch 2 times, most recently from 7e29543 to 5f71a80 Compare October 22, 2022 05:41
@Varriount Varriount merged commit ea0e45e into nim-lang:devel Oct 24, 2022
@github-actions
Copy link
Contributor

Thanks for your hard work on this PR!
The lines below are statistics of the Nim compiler built from ea0e45e

Hint: mm: orc; opt: speed; options: -d:release
164024 lines; 8.340s; 613.145MiB peakmem

capocasa pushed a commit to capocasa/Nim that referenced this pull request Mar 31, 2023
* Added 'openarray[char]' overloads to 'std/parseutils'

* Removed redundant `start` and `last` params from slice using procs

* Fixed type for parseIdent overload

* fixed one by off with 'substr'

* removed missed start parameters for procedures

* Added 'openarray[char]' overloads to 'std/parseutils'

* Removed redundant `start` and `last` params from slice using procs

* Fixed type for parseIdent overload

* fixed one by off with 'substr'

* removed missed start parameters for procedures

* Fixed VM op to work with new 'opcSlice'

* Corrected captureBetween's logic to work with openarray

* js sys's parsefloat logic now uses openarray

Co-authored-by: Clay Sweetser <[email protected]>
bung87 pushed a commit to bung87/Nim that referenced this pull request Jul 29, 2023
* Added 'openarray[char]' overloads to 'std/parseutils'

* Removed redundant `start` and `last` params from slice using procs

* Fixed type for parseIdent overload

* fixed one by off with 'substr'

* removed missed start parameters for procedures

* Added 'openarray[char]' overloads to 'std/parseutils'

* Removed redundant `start` and `last` params from slice using procs

* Fixed type for parseIdent overload

* fixed one by off with 'substr'

* removed missed start parameters for procedures

* Fixed VM op to work with new 'opcSlice'

* Corrected captureBetween's logic to work with openarray

* js sys's parsefloat logic now uses openarray

Co-authored-by: Clay Sweetser <[email protected]>
Araq pushed a commit that referenced this pull request Aug 12, 2024
narimiran pushed a commit that referenced this pull request Aug 19, 2024
…aram [backport] (#23941)

fixes #23936
follow up #20527

(cherry picked from commit b215ec3)
@ZoomRmc
Copy link
Contributor

ZoomRmc commented Sep 28, 2025

Closes #14810

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants