[PATCH] D36768: [test-suite] Add -i option to fpcmp to ignore whitespace changes.

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur marked an inline comment as done.
Meinersbur added inline comments.


================
Comment at: tools/fpcmp.c:260
+      // Without this check, we would be in an infinite loop.
+      if (!ignore_whitespace && isspace(*F1P) && isspace(*F2P))
+        return 1;
----------------
MatzeB wrote:
> MatzeB wrote:
> > You mustn't pass `char` to libc character test functions but must cast characters to `unsigned char` (this is so intuitive, isn't it?). I guess you could also fix the existing isspace uses in the file while on it...
> > 
> > I wonder if this isn't a case that should rather be handled in CompareNumber.
> Is the `if (!ignore_whitespace) { ... }` still necessary after your changes to `CompareNumbers`?
> I wonder if this isn't a case that should rather be handled in CompareNumber.

I'd avoid making `CompareNumbers` have side-effects if no number was found. There are already too many bugs in there. (whitespace changes before numbers have always been ignored; intentional?)


================
Comment at: tools/fpcmp.c:297-298
   if (A_size == 0 || B_size == 0) {
     fprintf(stderr, "%s File sizes differ\n", g_program);
     return 1;
   }
----------------
MatzeB wrote:
> Wouldn't we abort here when comparing an empty file with a file with just spaces even in `-i` mode?
fixed.


================
Comment at: tools/fpcmp.c:52
   // If we didn't stop in the middle of a number, don't backup.
-  if (!isNumberChar(*Pos) && !isNumberChar(Pos[-1])) return Pos;
   // If we're one past the number, the two numbers can have different number
----------------
`Pos[-1]` is an invalid access if backing-up from the first character in the file.


================
Comment at: tools/fpcmp.c:101
   // If we stop on numbers, compare their difference.
   if (!isNumberChar(*F1P) || !isNumberChar(*F2P)) {
     // The diff failed.
----------------
`F1P` and `F2P` might have reached the end of file already, after skipping the whitespace. In that case these accesses are out-of-bounds. (Thy would read the terminating '\0', arguably not a number char, but they get printed in the fprintf below)


================
Comment at: tools/fpcmp.c:114-117
       char StrTmp[200];
       memcpy(StrTmp, F1P, EndOfNumber(F1NumEnd)+1 - F1P);
       // Strange exponential notation!
       StrTmp[(unsigned)(F1NumEnd-F1P)] = 'e';
----------------
Possible stack overflow? Just have a long sequence of zeros (> 200 chars) in the file.


https://reviews.llvm.org/D36768