nim-lang / Nim

Nim is a statically typed compiled systems programming language. It combines successful concepts from mature languages like Python, Ada and Modula. Its design focuses on efficiency, expressiveness, and elegance (in that order of priority).
https://nim-lang.org
Other
16.62k stars 1.47k forks source link

streams.write doesn't work with openArray or seq type arguments #14544

Open demotomohiro opened 4 years ago

demotomohiro commented 4 years ago

Example

import streams

template testStream(arg: untyped): untyped =
  var ss = newStringStream()
  ss.write(arg)
  ss.flush()
  echo ss.data

let
  ary = ['N', 'i', 'm', ' ', 'l', 'a', 'n', 'g', 'u', 'a', 'g', 'e', '\0']
  sq = @ary
testStream(ary)
testStream(sq)

proc test(ary: openArray[char]) =
  testStream(ary)

test(ary)
test(sq)

Current Output

Nim language
P�P
Nim lang
Nim lang

Expected Output

Nim language
Nim language
Nim language
Nim language

Possible Solution

Add following overload to streams module.

proc write*[T](s: Stream, x: openArray[T] | seq[T]) =
  for v in x:
    s.write(v)

  when false:
    # Edit: Following code doesn't work correctly when T is seq
    if x.len != 0:
      writeData(s, unsafeAddr(x[0]), sizeof(T) * x.len)

Without proc write[T](s: Stream, x: seq[T]), proc write*[T](s: Stream, x: T) is called for a seq type argument and it doesn't work.

Alternative Solution

Forbid calling streams.write with openArray or seq. User must use streams.writeData or call streams.write for the each elements.

proc write*[T](s: Stream, x: openArray[T] | seq[T]) {.error.}

Additional Information

f:\temp>nim -v
Nim Compiler Version 1.3.5 [Windows: amd64]
Compiled at 2020-06-02
Copyright (c) 2006-2020 by Andreas Rumpf

git hash: e5b64af8317eb0f5e8b9912691b69aab4cd26adf
active boot switches: -d:release
timotheecour commented 4 years ago

i don't like proc write*[T](s: Stream, x: T) at least as currently written; it seems fundamentally buggy. How is it even supposed to work for types that have data on the heap like seq[T]:

proc write*[T](s: Stream, x: T) =
  writeData(s, unsafeAddr(x), sizeof(x))

this can't work. I think we should deprecate that proc

demotomohiro commented 4 years ago

How about to deprecate proc write*[T](s: Stream, x: T) and add following 2 overloaded streams.write so that all types except object/tuple/ptr/ref can be written with streams.write?

proc write*[T](s: Stream, x: T) {.deprecated: "Overload streams.write for your type".}

type BitWritableTypes* = Ordinal | set | float32 | float64
proc write*(s: Stream, x: BitWritableTypes) =
  writeData(s, unsafeAddr(x), sizeof(x))

proc write*(s: Stream, x: openArray) =
  #When x is an array of object/tuple and user don't define the overloaded `write` for it, s.write generate compile error.
  for v in x:
    s.write(v)

I think streams module should not overload write for object/tuple type classes because bitwise write(writeData) not always work for these types. Even if object/tuple types don't have ptr/ref fields, they might contain int type fields like window handle or OpenGL's buffer object that you don't want to write them to stream. User should write a overloaded write for user's object/tuple types even if proc writeData*(s: Stream, buffer: pointer, bufLen: int) works for it. If user called streams.write with the object that contain fields like window handle without implementing a appropriate overloaded write and it compiled without error, finding that bug can be difficult.

timotheecour commented 4 years ago

that's too pessimistic; I suggest instead only enabling write for types with no ptr/pointer/ref fields (recursively). But might as well do it in a separate module, it's not something so fundamental that it should be auto-imported via system; so I suggest:

users who need write for types containing ptr/pointer/ref can always use a (reusable) string buffer to stringify it as they wish and then simply call write(s, buffer)

note

calling write[T](f: File, x: T) multiple times is probably less efficient than doing a single aggregate write(f, buf: string) after stringifying all arguments into a reusable string buffer.

and even if you must write multiple times, this is probably just as good:

for i in 0..<n:
  write(f, i)

=>

var buf: string
for i in 0..<n:
  buf.setLen 0
  buf.addInt i
  write(f, buf)