proc test(x: cstring) = echo cast[int](x)
var str: string = nil
test(nil) # output 0 -> correct
test(str) # output 8 ??
I think this should be a bug.
I use "Nim Compiler Version 0.15.2 (2016-10-23) [Windows: i386]"
As far as I know, cstring is just a pointer to the char's array. The inner representation of the string type contains such kind of array, so the converter just passed the array pointer from the inner representation without any nil-checks to your test procedure.
Probably the converter should perform the nil-checks to avoid problems while calling C code.
@yglukhov I have to say, that I do agree with your pull request. a nil string should be NULL when converted to a cstring. I think the problem here is, that values of type string can be of no string at all (nil), and functions can be implemented that nil is a totally valid argument. I think for those functions c functions that support this behaviour, there should be the least amount of surprises (nil to NULL/nullptr conversion). Hopefully at some point in time strings are not nil but empty by default, but that is another issue on its own.
@Ward the string is internally a pointer to something like this struct NimString{ size_t length; size_t capacity; char data[capacity]; };. The cstring conversion currently just returns a pointer to data.
I don't think adding a nil check will become the performance bottleneck. Here is my test code:
import times
proc MulDiv*(P1: int32, P2: int32, P3: int32): int32 {.stdcall, discardable, dynlib: "kernel32", importc.}
proc test(s: cstring) =
# do someting, to make sure the loop will not be eliminated by the compiler
MulDiv(cast[int32](s), cast[int32](s), cast[int32](s))
var
s1: string
s2 = "abc"
count = 100000000
start: float
start = cpuTime()
for i in 1..count:
test(cast[cstring](0))
test(cast[cstring](-1))
echo cpuTime() - start, " (loop only)"
start = cpuTime()
for i in 1..count:
test(s1)
test(s2)
echo cpuTime() - start, " (default convert)"
start = cpuTime()
for i in 1..count:
test(cast[cstring](cast[uint](s1) + (if s1.isNil: 0 else: 8)))
test(cast[cstring](cast[uint](s2) + (if s2.isNil: 0 else: 8)))
echo cpuTime() - start, " (safe convert)"
start = cpuTime()
for i in 1..count:
test(if s1.isNil: nil else: s1)
test(if s2.isNil: nil else: s2)
echo cpuTime() - start, " (worst code)"
Output
3.33 (loop only)
3.504 (default convert)
3.476999999999999 (safe convert)
24.2 (worst code)
nim -d:release --opt:speed
0.855 (loop only)
1.107 (default convert)
1.138 (safe convert)
3.684 (worst code)
After optimization, there is no conditional jump in the asm output for safe convert:
mov edx, DWORD PTR _s1_121012_755421110
mov DWORD PTR _i_121068_755421110, ebx
cmp edx, 1
sbb eax, eax
not eax
and eax, 8
add eax, edx
Most people will use the WORST CODE to check nil, right? So in my opinion, add the check by compiler is clever.
Another solution is allowing a custom converter to replace the default one, For example:
converter stringToCString(s: string): cstring =
result = cast[cstring](cast[uint](s) + (if s.isNil: 0 else: 8))