Yes, please. The "< 0" vs. "== -1" is the only thing missing. In line 347
if (*filesize < 0)
it it probably best to actually write
if (*filesize == (unsigned long int)-1)
since in the current version an aggressively optimizing compiler might do the following steps:
filesize is declared to be unsigned long *filesize unsigned values can never be negative
the if statement can never trigger
compiler removes the error check (though usually at least generating a warning)
Uff, I see. "usual arithmetic conversion" is really murky to me. So I try to err on the side of not relying on it.
I think I would agree with this assessment if the conversion happened in in the if statement. However the actual code is:
static char *ReadFile(FILE *file, unsigned long *filesize)
*filesize = ftell(file);
if (*filesize < 0)
// do stuff
so the result of ftell (a long int) is converted to unsigned long int when it is assigned to *filesize. Since filesize is known to be "unsigned long int" the optimization seems valid to me (namely an unsigned int comparison with <0 will always fail).
The cast of -1 to unsigned long int that I added is just to avoid the warning about comparing signed and unsigned values.
*filesize does not need to be set to anything in case of failures since we call CCTK_Error anyway. (Edited): Not quite correct, or at least only in the same correct that several levels up in the call tree do we call CCTK_VWarn(0, ...). Not that it is a bad idea to set to an "invalid" value in that case, though maybe "-1" and making filesize a long int * would be better since the file could actually be empty.
this it still seems to compare the result of ftell to <0 instead of the documented value of -1. While I strongly doubt that in this particular way of coding this comparison, there will be a problem (since it would require that the standard uses another negative value, other than -1, to indicate a non-error return of ftell) I do not quite see why we would deliberately test for a condition that is (at least in principle even if not in practise) different from the one documented.
Probably yes. I just hope that changing to signed long does not create a host of new warnings about mixing singed and unsigned types in the remainder of the code. Sorry for being so picky about all of this, in particular since this is all a very minor part of the code (is it even still used by the piraha parser code?) and the discussion by now has taken much more time than writing the patch likely did. The original patch though I think did have an actual bug in the comparison of *filesize to <0, so the review, mostly anyway, served its intended purpose of catching bugs.
Would it be possible to update the pull request with the actual proposed change, please? Otherwise it seems to me that there is some danger of not quite agreeing on the same thing to commit.
I've changed the variable from unsigned long to long.
I have not changed the error tests to check against -1, since all negative values indicate errors, in the sense that our malloc(filesize) later on will fail for any negative file size, independent of whether the file size is -1 indicating an error in ftell, or whether ftell returns another negative number for some other reason.