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

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov marked an inline comment as done.
mizvekov 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)
----------------
Quuxplusone wrote:
> 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.
I changed the function name and made it return instead the candidate itself in case it was copy elisible, which allowed simplifying some other expressions afterwards.

But in answer to the points made in this thread: I will generally prefer the syntax which mostly matches the intended usage, making it easier to do the right thing and harder to do the wrong thing.

This function implements a fragment of the C++ standard text in a very specific context within clang. It is called from two places.
It's not a major piece of infrastructure or anything like that, to be concerned with unforeseen possible future uses :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99696