[PATCH] D75236: [APFloat] Overload unary operator-
Ehud Katz via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Mar 2 11:36:37 PST 2020
ekatz 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();
----------------
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.
================
Comment at: llvm/lib/IR/ConstantFold.cpp:989
case Instruction::FNeg:
- return ConstantFP::get(C->getContext(), neg(CV));
+ return ConstantFP::get(C->getContext(), -CV);
}
----------------
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D75236/new/
https://reviews.llvm.org/D75236
More information about the llvm-commits
mailing list