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

[Jay Foad via Phabricator via llvm-commits]

foad marked 4 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:
> foad wrote:
> > 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 [...]".
> You are correct, placing it after the `neg` does make sense, and the quoted advise makes much sense as well.
> However, in this case, not-making it a member function, creates inconsistency with the other operators, and especially, it looks confusing and strange to have 2 versions of the `operator-` that take a single argument: one as a member function and the other as non-member.
OK, done.


================
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:
> foad wrote:
> > 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-`!
> I agree, but in this case I would rather not change it, and maybe just use it for future uses.
> In particular, it makes more sense to keep using `neg` in this case, as it explicitly shows the connection to the `FNeg` enum.
> In addition, it is consistent with the `FAdd` (at line 1338), `FSub`, `FMul`, etc. as they are implemented using the explicit function and not the corresponding operator.
I'd prefer to remove neg altogether; see D75511. I'm keeping that as a separate patch to avoid a "flag day" change.

I'd prefer to simplify the code at line 1338 to use overloaded operators too. There is still a case for keeping add() etc as functions, because they take an additional rounding mode argument. There is no reason to keep neg() as it is very simple and takes no additional arguments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75236