[PATCH] D99696: [clang] NRVO: Improvements and handling of more cases.

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added inline comments.


================
Comment at: clang/lib/Sema/SemaStmt.cpp:3290
     }
-
-    if (!getLangOpts().CPlusPlus20 && NeedSecondOverloadResolution &&
-        !getDiagnostics().isIgnored(diag::warn_return_std_move,
+    if (!getDiagnostics().isIgnored(diag::warn_return_std_move,
                                     Value->getExprLoc())) {
----------------
mizvekov wrote:
> mizvekov wrote:
> > rsmith wrote:
> > > Should we skip this if `NRVORes.isMoveEligible() && getLangOpts().CPlusPlus20`? In that case, we already tried an unrestricted move a couple of lines ago, so we know that adding `std::move` won't help. (I think we can still hit the "adding `std::move` will help" case in C++20, but only in obscure corners such as a `volatile`-qualified local variable.)
> > This goes to the documentation on `getNamedReturnInfo`,  return value, where it says that the Canidate member will be non-null in case implicit move would be permitted under the most permissive language standard.
> > 
> > So if Try
> > 
> > So if we are guarded under that condition, we would never 
> Skipping it is just a small compilation performance optimization:
> 
> We are going to try that `TryMoveInitialization` again just below, this time with equivalent of `ForceCXX20 = true`. If we were already in c++20 mode, then that second call is going to be made with basically the same parameters and perform basically the same work and reach the same conclusion: Faliure, and the diagnostic won't be produced.
> 
> Now my take on why that looks confusing: To me `TryMoveInitialization` has a similar problem to the one I addressed in this patch for `getCopyElisionCandidate`, it relies on you calling it multiple times, once to try the the actual work, and if that fails, then you have to call it again, this time in 'permissive' more, to get the information you need for the diagnostics.
> 
> Now I did not try giving it the same treatment of returning an aggregate with all the information you need, so you can call it just once with one less parameter.
> If you agree on that take, I'd be happy to try a similar improvement there as well.
> If you don't think it's worth the noise, then it's all good, no worries :)
> 
> As a side note, I just noticed that the
> `if (QT->isLValueReferenceType()) {` below is dead code, we are never going to consider this move eligible in any mode, and so Res.Candidate is going to be null.
> 
> We should not need to repeat here the checks we already do in getNRVOResult, so I am going to go ahead and remove it.
> 
> I think it may have been added because until just recently `getCopyElisionCandidate` was bugged and accepted LValue references, and I guess the problem was "fixed" in this diagnostic first.
Disregard this comment, I realized I was answering the wrong question and for some reason it was not discarded.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99696