std/lists: Various changes to lists (RFC #303)

[salvipeter]

No description provided.

[salvipeter]

As discussed here.

[Clyybber]

append should not be deprecated IMO, as it looks more natural in code that also uses prepend.

[Araq]

Yeah, please don't deprecate append, I don't appreciate ecosystem-damaging micro-optimizations.

[salvipeter]

While I'm at it, there are a couple of other functions that would be helpful, such as:

• length
• isEmpty
• [] (indexing operator, giving the n-th element)
• first & rest (maybe pop and popFirst ?)

And some unification:

• no uppercase variables (L) - maybe a?
• in runnableExamples, always use from ... import (instead of just import)?
• in runnableExamples, always use assert (instead of doAssert)?
• in runnableExamples, always use toSeq (instead of $... == "...")?
• there is considerable variation in style, e.g., add(a, b) vs. a.add(b) vs. a.add b ... what is the policy?

What do you think? Also, is the reference to the list-list add in the reST comment of append (l. 379) correct? Can I specify it better?

[timotheecour]

While I'm at it, there are a couple of other functions that would be helpful, such as:

yes, but all these in another PR so that this PR can be merged sooner rather than later :)
in fact, 2 PRs: one for style-like changes, and one for symbol additions.

notes:

• length => should be len

• isEmpty => not sure; a.len == 0 would be less efficient indeed, but I'm wondering whether a completely generic isDefault would subsume this (see [RFC] add isDefault; more general than isNil, isEmpty etc #13526)

• no uppercase variables (L) - maybe a? yes

• [] yes (and also []=)

• first & rest; no; IMO it's better (more general) to split on a Node n: split(a: list, b: Node): tuple[lhs, rhs: list]

• (maybe pop and popFirst ?) I need to think; it's a bit redundant with remove; let's re-discuss this

• in runnableExamples, always use assert (instead of doAssert)? no, because there's no consensus yet on assert vs doAssert inside runnableExamples (only inside tests, where doAssert should be used)

• in runnableExamples, always use toSeq (instead of $... == "...")? yes, except of course for the runnableExamples for $

• in runnableExamples, always use from ... import (instead of just import)?
  EDIT: how about: only if there's a single import, so that examples are kept small; eg:
  from sequtils import toSeq : ok
  import sequtils, sugar: ok
  if/when syntax request: from std/[foo1,foo2] import nil ; from "." / [foo1,foo2] import bar1, bar2, bar3 RFCs#76 is implemented, we'd be able to have:
  from sequtils, sugar import collect, toSeq

there is considerable variation in style, e.g., add(a, b) vs. a.add(b) vs. a.add b ... what is the policy?

historical baggage and lack of specification / enforcement of https://nim-lang.github.io/Nim/nep1.html, let's discuss this separately

Also, is the reference to the list-list add in the reST comment of append (l. 379) correct? Can I specify it better?

we currently can't do better than what you wrote (lists.html#add,T,T) because docgen ignores the : SomeLinkedList in the declaration proc add[T: SomeLinkedList](a: var T; b: T) (so in particular would be incorrect with another overload proc add[T: Foo](a: var T; b: T)), so what you wrote is correct; however, the nim-lang/RFCs#125 I proposed will simplify all this and also make doc links more robust.

[timotheecour]

@salvipeter please remember to mark as resolved comments you've resolved :)

[timotheecour]

• @salvipeter for remove how about instead of a noop (or before your recent commit, a SIGSEGV), do as follows:

proc remove*[T](L: var SinglyLinkedList[T], n: SinglyLinkedNode[T]) =
  ## ...
=>
proc remove*[T](L: var SinglyLinkedList[T], n: SinglyLinkedNode[T]): bool =
  ## ...
  ## Returns true if `n` was found in `L`

which is more flexible, eg client code can discard the result if they don't care, of doAssert if they care that element was in list prior to remove.

Note that the pre-existing remove(DoublyLinkedList, n) is O(1) instead of O(n) and cannot do such a check, so returning void in that case was fine.

[timotheecour]

• proc append*[T](a: var SomeLinkedCollection[T], b: SomeLinkedNode[T] | T) = isn't correct as it'd accept this:

var a = [10,11,12,13].toDoublyLinkedList
a.append newSinglyLinkedNode[int](5)

(and then would give an error in the wrong place, inside instantiation of append instead of giving a sigmatch error before instantiating append)

instead do this:

proc append*[T](a: var (SinglyLinkedList[T] | SinglyLinkedRing[T]), b: SinglyLinkedNode[T] | T) =
  ## ...
  a.add b
proc append*[T](a: var (DoublyLinkedList[T] | DoublyLinkedRing[T]), b: DoublyLinkedNode[T] | T) =
  ## ...
  a.add b

I don't think concepts or generics are powerful enough to allow writing this using a single proc (maybe @Araq knows how?), but this would be easy with {.enableif.} (refs #12048):

proc append*[T](a: var SomeLinkedCollection[T]), b: SomeLinkedNode[T] | T) {.enableif: type(a.head) is type(b).} =
  a.add b

for eg, this fails:

type Bar[T] = object
  foo: int
proc fn[T](a: Bar[T], b: typeof(a.foo)) = discard # Error: undeclared field: 'foo'

[salvipeter]

Yes, this was bothering me, too - it would be nice if it could be written as a single proc, but for the time being I changed it as you recommended. Also added SinglyLinkedList / DoublyLinkedList as a possible type for b, which was missing.
To be consistent, we also need an appendMoved which is an alias for addMoved ... but it is a bit strange to introduce a new symbol and call it a "deprecated alias" right away.

[salvipeter]

As for remove returning a bool, I am not so sure. I think it has little use, but it would clutter up "normal" code with discards.

[timotheecour]

To be consistent, we also need an appendMoved which is an alias for addMoved

not sure we need; the point of re-adding append after the rename append=>add was to avoid breaking code; but no code ever used appendMoved so no need to introduce this alias

[timotheecour]

it would clutter up "normal" code with discards.

there's also {.discardable.}

[timotheecour]

ping @salvipeter

[salvipeter]

ping @salvipeter

I'm still alive, just work & family have priority :) I'll do a revision soon though.

[salvipeter]

JavaScript compilation seems to break because the swap a, b call at lists.nim:408 has no effect, I don't know why.

[timotheecour]

Suggested change

	proc appendMoved*[T: SomeLinkedList](a, b: var T) {.since: (1, 5, 1).} =
	## Alias for `a.addMoved(b)`.
	##
	## See also:
	## * `addMoved proc <#addMoved,SinglyLinkedList[T],SinglyLinkedList[T]>`_
	## * `addMoved proc <#addMoved,DoublyLinkedList[T],DoublyLinkedList[T]>`_
	a.addMoved b

refs #16536 (comment)

[salvipeter]

I think it would be a mistake not to add appendMoved, especially now that we are not deprecating the append functions at all. If there is prependMoved, symmetry asks for appendMoved. But that's just my 10 cents.

[timotheecour]

how about removing it in this PR and so that we can move forward with this PR and adding it in another PR so it can be discussed separately without block this?

apart from this point, I think it's ready to go

[salvipeter]

I'm not that emotionally attached to the idea, just wanted to give you a chance to reconsider :)

[Clyybber]

I agree with salvipeter, and it should be decided in this PR IMO.

[Clyybber]

@timotheecour I'm readding it to get on with this and not subject salvipeter to this review ping pong :D
Removing appendMoved can also be done in a further PR (until the release); but as of now there is no argument to not add it.

EDIT: Let's wait for @Araq to decide.

[timotheecour]

LGTM, excellent work!

[salvipeter]

ping ?

[timotheecour]

@salvipeter thanks for pinging, don't hesitate to in future, it's easy to forget PR's ; now that this was merged, PR welcome to address remaining points that were discussed here (ideally, breaking down in multiple smaller PR's whenever possible will make reviewing/merging faster)

[ringabout]

The regression devel docs

[timotheecour]

fixed in #16981; thanks @xflywind