On Wed, Feb 3, 2010 at 12:50 PM, Peter Wang
<novalazy@xxxxxxxxxx> wrote:
This is about fixing the flaw where al_fread16le, etc., may return
an error code EOF that clashes with a valid value (oops, again!)
It was suggested to change the prototype to:
bool al_fread16le(ALLEGRO_FILE *f, int32_t *value);
or
size_t al_fread16le(ALLEGRO_FILE *f, int32_t *value);
We would probably need four variants of each (signedness × endianness).
The current solution is this:
int32_t al_fread32be(ALLEGRO_FILE *f, bool *ret_success)
Due to implicit conversions we don't need unsigned variants.
Which option would you prefer?
We never need unsigned variants as far as I can see. al_fread16le(&x) seems to work without warning when x is unsigned.
Otherwise I kinda like the size_t variant as it goes with al_fread/al_fwrite. (Except, I still think we should not use size_t anywhere in the public API.)
But anyway, trying to write some actual code with these functions, it seems we should not add any extra parameter at all. Compare the snippet below, using the version returning the read bytes:
size_t s;
int w; s = al_fread16le(f, &w); if (s != 2) break;
int h; s = al_fread16le(f, &h); if (s != 2) break;
with the bool variant:
int w; if (!al_fread16le(f, &w)) break;
int h; if (!al_fread16le(f, &h)) break;
and with simply doing it like this:
int w = al_fread16le(f); if (feof(f)) break;
int h = al_fread16le(f); if (feof(f)) break;
Basically, I find the last version the most readable - especially when I'm not checking for end-of-file after each read item. So I'd say we should simply remove the extra parameter from the 32 bit versions again.