[PATCH] D27872: [APFloat] Switch from (PPCDoubleDoubleImpl, IEEEdouble) layout to (IEEEdouble, IEEEdouble)

[Eric Christopher via Phabricator via cfe-commits]

echristo added a comment.

Few comments inline. Generally looks OK, I do share Hal's comment on finding some way of simplifying the if (someSemantics) ... if (otherSemantics) ... llvm_unreachable(...) pattern.

What did you have in mind?



================
Comment at: llvm/include/llvm/ADT/APFloat.h:800
 
-  void makeLargest(bool Neg) { getIEEE().makeLargest(Neg); }
+  void makeLargest(bool Neg) {
+    if (usesLayout<IEEEFloat>(getSemantics())) {
----------------
mehdi_amini wrote:
> jtony wrote:
> > I know it is allowed to return a void function call inside a void function, but I think this reduces the code readability in general and causes some confusing to some people. Maybe it is better to avoid using this kind of coding style. I think we can simply call each function in each branch without the 'return' keyword, by default, the program will reach the end of function and return.  
> > 
> > One possible equivalent code:
> > 
> > void makeNaN(bool SNaN, bool Neg, const APInt *fill) {
> >     if (usesLayout<IEEEFloat>(getSemantics())) {
> >       U.IEEE.makeNaN(SNaN, Neg, fill);
> >     } else if (usesLayout<DoubleAPFloat>(getSemantics())) {
> >       U.Double.makeNaN(SNaN, Neg, fill);
> >     } else {
> >       llvm_unreachable("Unexpected semantics");
> >     }
> >   }
> Two reasons not to do that:
> 
> 1) It makes it less consistent with all the other cases, and consistency is a high criteria for readability
> 2) returning a function call in a void function makes it so that if the callee signature is changed to return something in the future, it can't be ignored here (i.e. it makes it less error prone somehow).
> 
> Alternatively, you can also write it this way:
> 
> ```
>   if (usesLayout<IEEEFloat>(getSemantics())) {
>     U.IEEE.makeNaN(SNaN, Neg, fill);
>     return;
>   } 
>   if (usesLayout<DoubleAPFloat>(getSemantics())) {
>     U.Double.makeNaN(SNaN, Neg, fill);
>     return;
>   } 
>   llvm_unreachable("Unexpected semantics");
> ```
I prefer the alternate way that Mehdi has it here rather than returning void if possible.


================
Comment at: llvm/include/llvm/ADT/APFloat.h:834
   cmpResult compareAbsoluteValue(const APFloat &RHS) const {
-    assert(&getSemantics() == &RHS.getSemantics());
+    assert(&getSemantics() == &RHS.getSemantics() &&
+           "Should only compare APFloats with the same semantics");
----------------
Go ahead and commit this separately. (And the rest of the assert changes to add descriptions of the assert).


================
Comment at: llvm/include/llvm/ADT/APFloat.h:1039
+  /// \brief Operator+ overload which provides the default
+  /// \c nmNearestTiesToEven rounding mode and *no* error checking.
   APFloat operator+(const APFloat &RHS) const {
----------------
What's with the no error checking. Also, unless my font kerning is terrible you have nmNearestTiesToEven versus rmNearestTiesToEven.


================
Comment at: llvm/lib/Support/APFloat.cpp:4040
+                                          APFloat::roundingMode RM) {
+  if (Semantics == &semPPCDoubleDouble) {
+    APFloat Tmp(semPPCDoubleDoubleImpl, bitcastToAPInt());
----------------
To reduce indentation I'd probably assert first? (And all below)


https://reviews.llvm.org/D27872