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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: include/llvm/Support/BranchProbability.h:131-133
     BranchProbability Prob(*this);
-    return Prob += RHS;
+    Prob += RHS;
+    return Prob;
----------------
Quuxplusone wrote:
> 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.
Could we do something the other way around?

Could op+ be implemented with either a value passed left parameter?

Alternatively - should this warning/fixes be restricted to types with non-trivial copy construction? Because I'm not sure this code would be made any worse by the code being as-is? I would imagine it might produce the same code with or without your change, once optimized?


================
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());
 
----------------
Quuxplusone wrote:
> 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?
Sure! Done in r347609


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54885