[PATCH] D54885: Assigning to a local object in a return statement prevents copy elision. NFC.

[Arthur O'Dwyer via Phabricator via llvm-commits]

Quuxplusone marked 2 inline comments as done.
Quuxplusone added inline comments.


================
Comment at: include/llvm/Support/BranchProbability.h:131-133
     BranchProbability Prob(*this);
-    return Prob += RHS;
+    Prob += RHS;
+    return Prob;
----------------
dblaikie wrote:
> Would an alternative fix for this be to write ref-qualified overloads of opX=, then use them like:
> 
>   return BranchProbability(*this) += RHS;
> 
> ? (or, I suppose, "BranchProbability Prob(*this); return std::move(Prob) += RHS;")
Yes, absolutely that would work; but it would be horribly un-idiomatic.
Also, some wise people say that every assignment operator should ideally be //lvalue//-ref-qualified, and you're [hypothetically] suggesting to add //rvalue//-ref-qualified versions purely to enable an un-idiomatic style... I don't think it would be a good idea.


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:7845
   if (ShouldInvert)
-    return Cmp = DAG.getNOT(dl, Cmp, Cmp.getValueType());
+    Cmp = DAG.getNOT(dl, Cmp, Cmp.getValueType());
 
----------------
dblaikie wrote:
> This looks like a good change regardless - the original phrasing is pretty weird, given that Cmp is going out of scope anyway, it could've been written as "return DAG.getNOT(...);" & skipped the assignment, but your solution here's more consistent with the surrounding code, I think
Agreed.
I don't have commit privs; could someone commit this for me?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54885