questionable `csize` definition in `system.nim`

[jangko]

I'm sure the story of csize is different from cstring.

Current definition

csize* {.importc: "size_t", nodecl.} = int
    ## This is the same as the type ``size_t`` in *C*.

what interesting is the comment claims csize has the same type as size_t in C.
in reality, when we take a look at C or CPP header file, especially stddef.h, they agree it should be an unsigned int.

CPP doc and GNU C doc also agree size_t should be unsigned int.

if csize is an int, it implies csize can be negative or cannot exceed high(int) which is questionable.

also if it is used in FFI, it can be a subtle bug, very difficult to track down.

[krux02]

I strongly agree here. csize should be uint. I remember I had a discussion about this with @Araq where I told him that we need to change this in order to be correct, but can't remember his arguments anymore. Maybe he can repeat them. I thought I had an issue about this open already, but aparently there isn't.

[Araq]

Sizes beyond 2GB on 32 bit machines never come up anyway and C's way of using unsigned for "Natural" is so broken that I don't want to replicate it.

[krux02]

We had this discussion internally already, but I repeat my points for the public here. I agree that size_t should have been a signed integer for C and C++. I also agree that C's way of using unsigned for Natural is broken. But in my opinion that is pretty much irrelevant here, because csize is not about that at all. It is only for wrapper code. And here it is important the csize is actually what size_t is in C, and not what you wish size_t should have been in C.

[Araq]

Cool but it is what it is and you ignored my actual argument which is "it simply doesn't come up in practice".

[alaviss]

From my understanding of that csize definition, it would mean that:

• In the generated C code, the type will be size_t.
• In nimvm, it will be int.

If that's the case, then we should just change it to uint, since IIRC we have proper range checking for uint now, so that would help catching csize < 0 bugs.

This will be helpful in practice as ISO C defines size_t as:

Unsigned integer type of the result of the sizeof operator.

So having csize changed into a type that errors on values < 0 at compile time will help eliminate problems at runtime.

[jangko]

it simply doesn't come up in practice

What about memory mapped file? it can easily exceed 2GB. And actually I found this discrepancy when I thought I can use csize in place of SIZE_T used in a memory mapped proc.

[alaviss]

Yes, you can use it in place of size_t because it is size_t, check the generated code for more information.

In your case, it's clear that size_t didn't fit the bill. You should use a bigger type like int64 instead.

[jangko]

Yes, you can use it in place of size_t because it is size_t, check the generated code for more information.

probably the codegen is correct, but when we are still in Nim side and not yet crossing to the C, the Nim compiler will treat it as an int.

In your case, it's clear that size_t didn't fit the bill. You should use a bigger type like int64 instead.

It's an OS proc, I cannot choose how it is implemented.

[Araq]

Yet you can use a different type in your wrapper.

[alehander92]

but i agree with @jangko

What about memory mapped file? it can easily exceed 2GB.

[Araq]

Sorry, would break too much code for no gain. Rejected.