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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
