[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;
----------------
Meinersbur wrote:
> 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?)
No behaviour in `CompareNumbers` was changed. I just extracted the while loops into the `skip_whitespace` function which then can be called in multiple places.


================
Comment at: tools/fpcmp.c:56
   // comparison should have failed anyway.
-  if (!isNumberChar(*Pos)) Pos--;
 
----------------
This is probably the line causing the infinite loop. 

If we are in both, F1 and F2, just one char past an equal number (and the char-wise comparison does not advance because in my case, the following is LF and CR in the other). BackupNumber hence rewinds the entire number in F1 and F2, which is compared equal again, and the loop repeats at the CR/LF again. Two files containing "2a" and "2b" (equal numbers but different non-numeric characters following) trigger this already.

This means that this patch is not a complete fix for this phenomenon (I initially thought this happens only with whitespace, the aforementioned example loops as well). It is probably intended to match `0.2` and `0.20` as equal, but the assumption ("should") in the comment is definitely wrong.

I don't know a fix for this at the moment without breaking the use case.


https://reviews.llvm.org/D36768