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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
