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

deprecate API's with string/seq + start/last in favor of ones with openArray #381

Open
timotheecour opened this issue Jun 4, 2021 · 3 comments

Comments

@timotheecour
Copy link
Member

timotheecour commented Jun 4, 2021

proposal

benefits

  • simpler APIs (less overloads needed) and separation of concern (callee doesn't need to care where the slice came from), less code bloat
  • no broken edge cases (see example 2 below), no design by convention
  • more flexible

example 1

proc findBounds*(input: openArray[char], pattern: Regex, matches: var openArray[Slice[int]],
  start = 0): tuple[Slice[int]]

which would subsume all others and also be strictly more flexible, and less error prone

refs nim-lang/Nim#18028 (comment)

example 2

(refs nim-lang/Nim#18173)
optional start/last parameters lead to bad API's where a valid value ends up hijacked with a different meaning, eg:

func find*(a: SkipTable, s, sub: string, start: Natural = 0, last = 0): int

here, when user specifies last = 0, it ends up meaning last = sub.high, so there's no way for user to specify the edge condition of last = 0, which should be a valid choice

changing the signature to func find*(a: SkipTable, s, sub: string, start: Natural = 0, last = -1): int would be an improvement but is still unsatisfactory, as this could equally be interpreted as meaning s.high or empty slice of s (EDIT: func find*(a: SkipTable, s, sub: string, start: Natural = 0, last = sub.high): int would be possible too)

instead, func find*(a: SkipTable, s: openArray[char], sub: string): int would be self-documenting and not have any edge cases, the user then would call simply toOpenArray(s, start, end) if they want a slice

@Araq
Copy link
Member

Araq commented Jun 5, 2021

here, when user specifies last = 0, it ends up meaning last = sub.high, so there's no way for user to specify the edge condition of last = 0, which should be a valid choice

Well but Nim allows for

func find*(a: SkipTable, s, sub: string, start: Natural = 0, last = sub.high): int

it's just that the stdlib was written before this feature existed (and I personally don't like the feature).

It's a minor point, but RFCs should be correct. :-)

Regarding the RFC itself, IMO we can simply write new code with openArray and leave the old code untouched, I don't enjoy deprecating stuff that works well. Yes really, it does work well -- you can use Nim as a better Basic thanks to strutils and openArray soon enough will bite back with the required ownership rules.

@timotheecour
Copy link
Member Author

timotheecour commented Jun 5, 2021

It's a minor point, but RFCs should be correct. :-)

yes, I've updated RFC and included that sub.high as alternative

leave the old code untouched, I don't enjoy deprecating stuff that works well.

except that old code tends to not use sub.high and falls into "example 2" category, causing edge conditions for otherwise valid params (eg nim-lang/Nim#18173, there are/were other similar cases). Also uniformity in APIs (when applied to best practices) is still a good thing.

So we can still introduce the openArray overload for old APIs (with the special care described in nim-lang/Nim#18173 (comment) to ensure the openArray overload is selected when stard/end is unspecified), and whether to deprecate/soft-deprecate (with just a doc comment)/no-deprecate is secondary, at least new code can use old APIs using new style.

soon enough will bite back with the required ownership rules

do you have an example?

@Araq
Copy link
Member

Araq commented Jun 6, 2021

do you have an example?

You cannot write proc echo(x: varargs[openArray[char]]). Now you can argue that this should be allowed, as you can for all the other cases I could come up with, but this means we need to refine first-class openArray support first, make it non-experimental etc -- before we can build new APIs on top of it.

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

No branches or pull requests

2 participants