- changed status to open
- removed comment
Make ParseFile.c more robust
We don't check for all the errors yet.
Keyword:
Comments (17)
-
-
- changed status to open
- removed comment
-
- changed status to open
- removed comment
The error check is incorrect. fseek returns "-1" on error not "a negative number". This really only matters once we encounter a parfile that is >2GB in size, but still.
-
- removed comment
Roland, I'm confused. Fseek only returns 0 or -1. What difference should it make whether the file is bigger than 2GB?
-
- removed comment
Right, sorry. I was thinking of ftell. So the 2GB does not apply. Would still be nice to check for the documented return code.
-
- removed comment
All right, if we change "< 0" to "== -1" can we move this to review_ok?
-
- removed comment
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) -
reporter - removed comment
That's not a valid optimization. The "usual arithmetic conversions" mean that both LHS and RHS of the comparison operator are converted to unsigned before the comparison https://wiki.sei.cmu.edu/confluence/display/c/INT02-C.+Understand+integer+conversion+rules.
-
- removed comment
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.A test code:
void foo() { unsigned int bar = 1; if(bar < 0) { baz(); } if(bar == -1) { baz(); } }
gives me two warnings on gcc 7.3.0 when using -Wextra (but not eg -Wall):
rhaas@ekohaes8:.../rhaas/tmp$ gcc -Wall -Wextra -c compare.c compare.c: In function ‘foo’: compare.c:4:10: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits] if(bar < 0) { ^ compare.c:8:10: warning: comparison between signed and unsigned integer expressions [-Wsign-compare] if(bar == -1) { ^~
Note the first warning about the comparison always being false.
-
- removed comment
Why don't we just add a variable?
auto fresult = ftell(...) if(fresult < 0) { *filesize = 0; fprint(fstder,...) ... } else { *filesize = fresult; }
-
- removed comment
Not sure how good that actually is
*filesize
does not need to be set to anything in case of failures since we callCCTK_Error
anyway. (Edited): Not quite correct, or at least only in the same correct that several levels up in the call tree do we callCCTK_VWarn(0, ...)
. Not that it is a bad idea to set to an "invalid" value in that case, though maybe "-1" and makingfilesize
along 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.
-
- removed comment
OK, how about we change
ReadFile(FILE *file,signed long *filessize)
toReadFile(FILE *file,signed long *filesize)
and the<0
to== -1
. Can we then move to review_ok? -
- removed comment
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.
-
- removed comment
Yes, this source still gets called. It reads the buffer that Piraha parses.
-
- removed comment
Replying to [comment:14 sbrandt]:
Yes, this source still gets called. It reads the buffer that Piraha parses. I see. Thank you.
-
reporter - removed comment
-
- removed comment
Erik, this branch is owned by you rather than cactuscode. Can you change and commit?
- Log in to comment