[PATCH] D25977: [APFloat] Add DoubleAPFloat mode to APFloat. NFC.

[Hal Finkel via llvm-commits]

hfinkel accepted this revision.
hfinkel added a comment.
This revision is now accepted and ready to land.

Some grammar,etc. nits, otherwise LGTM.



================
Comment at: llvm/include/llvm/ADT/APFloat.h:569
 
+  /// Notice: this must be the first data member.
   /// The semantics that this value obeys.
----------------
s/Notice/Note/


================
Comment at: llvm/include/llvm/ADT/APFloat.h:600
+// This mode implements more precise float in terms of two APFloats.
+// The interface and layout is designed for arbitray underlying semantic,
+// though currently only PPCDoubleDouble semantic is supported, whose
----------------
semantic -> semantics


================
Comment at: llvm/include/llvm/ADT/APFloat.h:601
+// The interface and layout is designed for arbitray underlying semantic,
+// though currently only PPCDoubleDouble semantic is supported, whose
+// corresponding underlying semantic is IEEEdouble.
----------------
semantic is -> semantics are


================
Comment at: llvm/include/llvm/ADT/APFloat.h:602
+// though currently only PPCDoubleDouble semantic is supported, whose
+// corresponding underlying semantic is IEEEdouble.
+class DoubleAPFloat final : public APFloatBase {
----------------
semantic is -> semantics are

(semantics are always plural, unless you're taking about one particular property in particular - here, we're always talking about all of the properties that define a particular kind of floating-point number).


================
Comment at: llvm/include/llvm/ADT/APFloat.h:604
+class DoubleAPFloat final : public APFloatBase {
+  // Notice: this must be the first data member.
+  const fltSemantics *Semantics;
----------------
Notice -> Note


================
Comment at: llvm/include/llvm/ADT/APFloat.h:723
+    static_assert(std::is_same<T, IEEEFloat>::value ||
+                      std::is_same<T, DoubleAPFloat>::value,
+                  "");
----------------
I think this would look better if you lined up the 'std::is_same's vertically. 


================
Comment at: llvm/include/llvm/ADT/APFloat.h:724
+                      std::is_same<T, DoubleAPFloat>::value,
+                  "");
+    if (std::is_same<T, DoubleAPFloat>::value) {
----------------
This should fit on the previous line.


================
Comment at: llvm/lib/Support/APFloat.cpp:80
+
+  /* This is a transient semantics for the real PPCDoubleDouble implementation.
+     Currently, APFloat of PPCDoubleDouble holds one PPCDoubleDoubleImpl as the
----------------
This is a transient semantics for -> These are temporary semantics for


================
Comment at: llvm/lib/Support/APFloat.cpp:83
+     high part of double double, and one IEEEdouble as the low part, so that
+     the old operations operates on PPCDoubleDoubleImpl, while the newly added
+     operations also populate the IEEEdouble.
----------------
operates -> operate


================
Comment at: llvm/lib/Support/APFloat.cpp:86
+
+     TODO: Once all functions support DoubleAPFloat mode, we change all
+     PPCDoubleDoubleImpl to IEEEdouble and remove PPCDoubleDoubleImpl.  */
----------------
we -> we'll


https://reviews.llvm.org/D25977