APFloatTest: convert test accidentally ended up with x87DoubleExtended -> x87DoubleExtended noops.

[eddyb]

It looks like back in 2013 (b361adb), tests were added for these conversions:

IEEEsingle -> x87DoubleExtended	x87DoubleExtended -> x87DoubleExtended
APFloat::getSNaN(APFloat::IEEEsingle)	APFloat::getSNaN(APFloat::x87DoubleExtended)
APFloat::getQNaN(APFloat::IEEEsingle)	APFloat::getQNaN(APFloat::x87DoubleExtended)

The first column (IEEEsingle NaNs) makes sense, and it fits with the commit description:

APFloat: Make sure that we get a well-formed x87 NaN when converting from a smaller type.

But the second column looks like a mistake - the simplest explanation I can think of is they were meant to be IEEEdouble (which is still smaller than x87DoubleExtended, and so it would also fit the commit description) not noops.

cc @d0k

[d0k]

It's hard to guess what I did 10 years ago, might be that I wanted to make sure we don't modify the bits on a noop convert. Is this test causing trouble somehow?

[eddyb]

might be that I wanted to make sure we don't modify the bits on a noop convert

Sadly there's a kind of "two wrongs make a right" situation going on here:

1. the test is testing nothing, same-semantic APFloat::converts are always noops, regardless of semantics:
   

   llvm-project/llvm/lib/Support/APFloat.cpp

   Lines 5157 to 5162 in 97678ce

   	APFloat::opStatus APFloat::convert(const fltSemantics &ToSemantics,
   	roundingMode RM, bool *losesInfo) {
   	if (&getSemantics() == &ToSemantics) {
   	*losesInfo = false;
   	return opOK;
   	}

   • even if testing that was intended, the test should probably also be testing Double anyway, just like it is testing Single

2. IEEEFloat::convert on SNaNs is nowadays never a noop, because it sets the quiet bit - so if not for the APFloat::convert early return, the test would have had to been modified just like the one which converts a Single SNaN to x87DoubleExtended, back in 149f5b5 (link to the relevant part of the diff)

---

Is this test causing trouble somehow?

Nothing that significant, it just confused me for long enough (think about what happens if you remove the early-return from APFloat::convert and try to run the tests - the only one that will fail will be this one, though there aren't many convert tests to begin with) to make me suspect, once I understood what was happening, that it was a mistake originally, and open this issue, instead of working around it on my end.

The more specific context is that I was updating the C++ -> Rust port of APFloat (long story I'd rather not get into on here, maybe on IRC etc.) and applying 149f5b5 caused that test to fail, because the Rust port doesn't have an if (fromSemantics == toSemantics) return; equivalent.
_{(while it's supposed to match behaviorally, the Rust port uses static types in a few places where the original C++ code used dynamic values, and Semantics is the main one, and while comparing the from/to types isn't impossible, it felt unnecessary, preferring instead to treat same-type conversions generically - as if e.g. it was actually a roundtrip through a higher-precision format)}