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

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

Quuxplusone added inline comments.


================
Comment at: clang/lib/Sema/SemaStmt.cpp:3140
+/// \param ReturnType This is the return type of the function.
+void Sema::updNRVOResultWithRetType(NRVOResult &Res, QualType ReturnType) {
+  if (!Res.Candidate)
----------------
mizvekov wrote:
> aaronpuchert wrote:
> > mizvekov wrote:
> > > aaronpuchert wrote:
> > > > `NRVOResult` seems to be small, why not make this a proper function and let it return the result?
> > > It does use the result parameter as input and output. This function can only "downgrade" a result it received previously.
> > > I could make it receive a result and return the result, but then would probably just use it like this:
> > > ```
> > > NRVORes = updNRVOResultWithRetType(NRVORes, ...);
> > > ```
> > > Do you think that looks clearer?
> > Yes, that would seem more natural to me. Technically a caller could decide to not use the same variable in both places, for example it could pass a temporary or pass the result on directly.
> But for this function, I don't see how it could make sense to actually use it like that.
> 
> You are either using it in a context where you don't have a ReturnType (throw, co_return), and you only care about the initial NRVOResult and don't call this function at all, or you are in a return statement, where you will call this function passing the initial NRVOResult, which will then have no further utility.
I haven't looked at the code, but FWIW, I'm always in favor of more "function-style" functions. Which would you rather deal with — `void square(int& x)` or `int square(int x)`? The same rationale applies uniformly everywhere. Even if we are "consuming" the argument in this particular instance, that might change after another few years of maintenance; and regardless, it might be easier for the maintainer to understand "function-style" code even if they aren't going to change it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99696