This is an archive of the discontinued LLVM Phabricator instance.

[APFloat] Overload comparison operators
ClosedPublic

Authored by foad on Feb 27 2020, 3:53 AM.

Details

Summary

These implement the usual IEEE-style floating point comparison
semantics, e.g. +0.0 == -0.0 and all operators except != return false
if either argument is NaN.

Diff Detail

Event Timeline

foad created this revision.Feb 27 2020, 3:53 AM
arsenm added inline comments.Feb 27 2020, 6:03 AM
llvm/unittests/ADT/APFloatTest.cpp
4262–4263

This should probably be as before, with separate tests for the full range of operators

jfb added a reviewer: scanon.Feb 27 2020, 9:36 AM
ekatz added a comment.Mar 2 2020, 10:20 AM

I like this change!
However, I think (as @arsenm) that the tests shouldn't be changed, but add separate new ones for the newly overloaded operators.

foad updated this revision to Diff 248669.Mar 6 2020, 1:53 AM

Add unit tests instead of changing existing tests.

foad marked an inline comment as done.Mar 6 2020, 2:01 AM
ekatz added a comment.Mar 6 2020, 3:26 AM

Generally it LGTM.
But this change may be split into 2 as well (like the neg change).

I know that some times you see patches that change and add a lot of code (in the same patch), while in other cases they split into smaller ones. Both approaches are valid, but I think the better one is to split (if possible); especially in cases (like this) that change apply to files, from separate projects/modules, outside the one in which the new API is added.

So, I suggest split it into 2, where one adds the new operators and tests; and the other changes the compares in the different locations.

foad updated this revision to Diff 248725.Mar 6 2020, 7:24 AM

Split changes to callers out into a separate patch.

ekatz accepted this revision.Mar 6 2020, 7:46 AM

LGTM

This revision is now accepted and ready to land.Mar 6 2020, 7:46 AM
This revision was automatically updated to reflect the committed changes.