[PATCH] D102671: [APFloat] convertToDouble/Float can work on shorter types

[Serge Pavlov via Phabricator via llvm-commits]

sepavloff added inline comments.


================
Comment at: llvm/unittests/ADT/APFloatTest.cpp:4715
+
+TEST(APFloatTest, ToDouble) {
+  APFloat D1(0.0);
----------------
RKSimon wrote:
> Maybe break this into shorter tests "HalfToDouble", "BFloatToDouble", "FloatToDouble" etc, to help avoid the variable mismatches noticed by @foad ?
Now meaningful names are used.


================
Comment at: llvm/unittests/ADT/APFloatTest.cpp:4719
+  APFloat D2(-0.0);
+  EXPECT_EQ(-0.0, D1.convertToDouble());
+  APFloat D3(1.0);
----------------
foad wrote:
> This should be D2? So maybe use something stronger than EXPECT_EQ, that distinguishes +0 from -0?
> 
> You've made the same typo below for B2, H2, etc.
Thank you for the catch. Now meaningful names are used for the variables, it must help avoiding such errors.


================
Comment at: llvm/unittests/ADT/APFloatTest.cpp:4737-4738
+  APFloat D10 = APFloat::getInf(APFloat::IEEEdouble());
+  EXPECT_TRUE(std::isinf(D10.convertToDouble()));
+  EXPECT_TRUE(0 < D10.convertToDouble());
+  APFloat D11 = APFloat::getInf(APFloat::IEEEdouble(), true);
----------------
foad wrote:
> Instead of two EXPECT_TRUE checks can't you use `EXPECT_EQ(std::numeric_limits<double>::infinity(), D10.convertToDouble())` ?
The checks changed to use `EXPECT_EQ`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102671/new/

https://reviews.llvm.org/D102671