[PATCH] D29109: [APFloat] Fix comments. NFC.

[Justin Lebar via Phabricator via llvm-commits]

jlebar added inline comments.


================
Comment at: llvm/include/llvm/ADT/APFloat.h:1054
 
+  // TODO: bool parameters are not readable and one source of bugs.
+  //       Do something.
----------------
s/one/a/


================
Comment at: llvm/include/llvm/ADT/APFloat.h:1055
+  // TODO: bool parameters are not readable and one source of bugs.
+  //       Do something.
   opStatus next(bool nextDown) {
----------------
Nit, this indentation seems noncanonical.


================
Comment at: llvm/include/llvm/ADT/APFloat.h:1064
 
-  /// \brief Operator+ overload which provides the default
-  /// \c rmNearestTiesToEven rounding mode and *no* error checking.
+  /// Add two APFloats using \c rmNearestTiesToEven rounding mode.
+  /// *No* error checking.
----------------
Perhaps "Add two APFloats, rounding ties to the nearest even.", so we repeat less of what's in the code?


================
Comment at: llvm/include/llvm/ADT/APFloat.h:1065
+  /// Add two APFloats using \c rmNearestTiesToEven rounding mode.
+  /// *No* error checking.
   APFloat operator+(const APFloat &RHS) const {
----------------
Nit, I'm not sure we need the asterisks around "No".


================
Comment at: llvm/lib/Support/APFloat.cpp:69
 
-  /* The PowerPC format consists of two doubles.  It does not map cleanly
-     onto the usual format above.  It is approximated using twice the
-     mantissa bits.  Note that for exponents near the double minimum,
-     we no longer can represent the full 106 mantissa bits, so those
-     will be treated as denormal numbers.
-
-     FIXME: While this approximation is equivalent to what GCC uses for
-     compile-time arithmetic on PPC double-double numbers, it is not able
-     to represent all possible values held by a PPC double-double number,
-     for example: (long double) 1.0 + (long double) 0x1p-106
-     Should this be replaced by a full emulation of PPC double-double?
+  /* The IBM double-double numbers. It consists of a pair of IEEE 64-bit
+     doubles (Hi, Lo), where |Hi| > |Lo|, and (double)(Hi + Lo) == Hi.
----------------
s/It consists/They consist/ to match "number*s*" earlier.


================
Comment at: llvm/lib/Support/APFloat.cpp:70
+  /* The IBM double-double numbers. It consists of a pair of IEEE 64-bit
+     doubles (Hi, Lo), where |Hi| > |Lo|, and (double)(Hi + Lo) == Hi.
+     The numeric value it's modeling is Hi + Lo. Therefore it has two 53-bit
----------------
Well, that second criterion is only true for non-denormals, right?


================
Comment at: llvm/lib/Support/APFloat.cpp:72
+     The numeric value it's modeling is Hi + Lo. Therefore it has two 53-bit
+     mantissa parts that aren't necessarily adjacent with each other, and an
+     11-bit exponent.
----------------
adjacent to


================
Comment at: llvm/lib/Support/APFloat.cpp:82
      IBM double-double, if the accurate semPPCDoubleDouble doesn't handle the
-     case. It's equivalent to have an IEEE number with consecutive 106 bits of
-     mantissa, and 11 bits of exponent. It's not accurate, becuase the two
-     53-bit mantissa parts don't actually have to be consecutive,
-     e.g. 1 + epsilon.
+     operation. It's equivalent to having an IEEE number with 106 consecutive
+     bits of mantissa and 11 bits of exponent.
----------------
(Here "it's" is fine, because the antecedent is "semantics", used in a singular sense, rather than "numbers".)


================
Comment at: llvm/lib/Support/APFloat.cpp:4243
   auto Result = Floats[0].compare(RHS.Floats[0]);
+  // |Float[0]| > |Float[1]|
   if (Result == APFloat::cmpEqual)
----------------
Hm, in order for this to be true for denormals, they must also have the same sign, right?


https://reviews.llvm.org/D29109