SDL_RWRead returning errors?

[icculus]

Right now, SDL_RWRead (and others) will return 0 on EOF or error, with no good way to differentiate...the best practice is probably to clear the current SDL error, call SDL_RWRead, and if it's zero, see if SDL_GetError() returns a non-NULL result.

Since we're breaking API for SDL3, it might be time to change this from the stdio fread() style to the POSIX read() style: instead of reading X objects of Y size, we merely read X bytes, returning the number of bytes read, zero at EOF, and -1 on error...this also makes it easy to decide what to do if we only read half an object, since it's just a pile of bytes that can simply be less bytes instead of some fraction of an object.

[slouken]

Yes please! :)

[icculus]

This is in, and sdl2-compat is updated to handle it in https://github.com/libsdl-org/sdl2-compat/commit/8d19a3a46586f7cfa1517ddd04c452a8cea44d4e

[slouken]

There is potentially an issue dropping this in as a replacement for OS APIs that use size_t. Previously they would just work, now each return value and parameter needs a cast to and from size_t.

[icculus]

(Doh, I didn't build the tests, I'll fix that up shortly.)

[icculus]

There is potentially an issue dropping this in as a replacement for OS APIs that use size_t.

Like this?

    if (using_sdl) {
        readfn = SDL_RWread;
    } else {
        readfn = fread;
    }

[slouken]

Well, fread is a bad example since it has a different parameter set now, but like read(), yes.

[sezero]

There is potentially an issue dropping this in as a replacement for OS APIs that use size_t.

There already is at least one incompatibility with stdio, i.e. RWseek and fseek having different semantics. With that in place, I wouldn't mind the new incompatibility.

Like this?

    if (using_sdl) {
        readfn = SDL_RWread;
    } else {
        readfn = fread;
    }

Anyone doing that is looking for trouble anyway.

[icculus]

I mean, one being SDLCALL already causes potential problems here...

[icculus]

Thinking more on this, it doesn't really make sense to use an Sint64 here, since you can't read or write more than your address space can hold in a single call, even if the file itself is > 0xFFFFFFFF bytes in size.

I do need a signed data type, though...is ssize_t safe to use?

[slouken]

Thinking more on this, it doesn't really make sense to use an Sint64 here, since you can't read or write more than your address space can hold in a single call, even if the file itself is > 0xFFFFFFFF bytes in size.

I do need a signed data type, though...is ssize_t safe to use?

It seems like the right thing to use, though I haven't seen it used much in practice. What do the current C++ standard file I/O routines use?

[sezero]

I do need a signed data type, though...is ssize_t safe to use?

IIRC, ssize_t isn't standard, at least MSVC doesn't have it.

[icculus]

Should we add a SintPtr? :)

[sezero]

Should we add a SintPtr? :)

If you really want to :) But it's not related to [s]size_t at all.

[slouken]

Should we add a SintPtr? :)

No :)

[icculus]

But it's not related to [s]size_t at all.

I'm worried I'm about to discover I've believed something incorrect for years but...isn't size_t meant to be an integer that matches the size of an application's address space?

[slouken]

This is a great answer about size_t: https://stackoverflow.com/a/55204639

[sezero]

Also see https://github.com/libsdl-org/SDL/pull/4552 about size_t vs intptr_t

[slouken]

I just did a bunch of refactoring code to use the new API, and Sint64 hasn't been a big problem. I'm going to go ahead and close this for now.

[slouken]

Upon further real-world usage, constantly casting to and from Sint64 has been a real pain.

Instead I'm changing the functions to use size_t again, and adding a status field so callers can detect the difference between end of file and a read error if they care.