[PATCH] D75236: [APFloat] Overload unary operator-

[Jay Foad via Phabricator via llvm-commits]

foad marked 3 inline comments as done.
foad added inline comments.


================
Comment at: llvm/include/llvm/ADT/APFloat.h:1246
+/// Returns the negated value of the argument.
+inline APFloat operator-(APFloat X) {
+  X.changeSign();
----------------
ekatz wrote:
> Why not make this a member of `APFloat`, just like the other operator overloads?
Sure, I could do, but I don't see a compelling reason to do that and I thought it was simpler to follow the example of existing `neg`.

Others would advise me to make the other operator overloads non-members instead. For example https://en.cppreference.com/w/cpp/language/operators says "Binary operators are typically implemented as non-members [...]".


================
Comment at: llvm/lib/IR/ConstantFold.cpp:989
     case Instruction::FNeg:
-      return ConstantFP::get(C->getContext(), neg(CV));
+      return ConstantFP::get(C->getContext(), -CV);
     }
----------------
ekatz wrote:
> Why change it? Calling `operator-()` seems a bit more elusive, than the `neg`.
> (This also regards to the changes in "TargetLowering.cpp".)
I think that's a matter of taste. If I didn't prefer `-CV` to `neg(CV)` then I wouldn't have bothered to implement `operator-`!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75236/new/

https://reviews.llvm.org/D75236