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

Mehdi AMINI via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Dec 19 18:29:02 PST 2016


mehdi_amini added inline comments.


================
Comment at: llvm/include/llvm/ADT/APFloat.h:800
 
-  void makeLargest(bool Neg) { getIEEE().makeLargest(Neg); }
+  void makeLargest(bool Neg) {
+    if (usesLayout<IEEEFloat>(getSemantics())) {
----------------
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");
```


https://reviews.llvm.org/D27872





More information about the cfe-commits mailing list