ChuanqiXu accepted this revision.
ChuanqiXu added inline comments.
This revision is now accepted and ready to land.
================
Comment at: clang/lib/Sema/Scope.cpp:152-154
+ // Consider the variable as NRVO candidate if the return slot is available
+ // for it in the current scope, or if it can be available in outer scopes.
+ NRVO = CanBePutInReturnSlot ? VD : nullptr;
----------------
rusyaev-roman wrote:
> ChuanqiXu wrote:
> > rusyaev-roman wrote:
> > > ChuanqiXu wrote:
> > > > What if NRVO contains a value already? It is possible due to the value
> > > > of NRVO could be set by its children.
> > > Actually this is intention. If the parent has already NRVO candidate,
> > > then it should be invalidated (or not). Let's consider the following
> > > examples:
> > >
> > >
> > > ```
> > > X foo(bool b) {
> > > X x;
> > > X y;
> > > if (b)
> > > return x;
> > > else
> > > return y; // when we process this return statement, the parent has
> > > already NRVO and it will be invalidated (this is correct behavior)
> > > }
> > > ```
> > >
> > > ```
> > > X foo(bool b) {
> > > X x;
> > > if (b)
> > > return x;
> > >
> > > X y;
> > > // when we process this return statement, the parent has already NRVO
> > > and it WON't be invalidated
> > > // (this is correct behavior), because a return slot will be
> > > available for it
> > > return y;
> > > }
> > > ```
> > >
> > > ```
> > > X foo(bool b) {
> > > X x;
> > > if (b)
> > > return x;
> > >
> > > // when we process this return statement, the parent has already NRVO
> > > and it WON't be invalidated (this is correct behavior)
> > > return x;
> > > }
> > > ```
> > >
> > > ```
> > > X foo(bool b, X x) {
> > > X y;
> > >
> > > if (b)
> > > return x;
> > >
> > > // when we process this return statement, the parent contains nullptr
> > > (invalid candidate) and it will be invalidated (this is correct behavior)
> > > return y;
> > > }
> > > ```
> > >
> > > ```
> > > X foo(bool b, X x) {
> > > if (b)
> > > return x;
> > >
> > > X y;
> > > // when we process this return statement, the parent contains nullptr
> > > (invalid candidate) and it WON't be invalidated (this is correct behavior)
> > > return y;
> > > }
> > > ```
> > Oh, I see. Tricky. I don't find invalid cases now. But I recommend to
> > comment that the children would maintain the `ReturnSlots` of their
> > parents. (This is anti-intuition)
> >
> > Have you tested any larger projects? Like libc++, libstdc++ or something
> > like folly. I feel we need to do such tests to avoid we get anything wrong.
> I've already added a comment at the beginning of `updateNRVOCandidate`
> function where this point is mentioned:
> ```
> // ... Therefore, we need to clear return slots for other
> // variables defined before the current return statement in the current
> // scope and in outer scopes.
> ```
> If it's not enough, please let me know.
>
>
> > Have you tested any larger projects?
>
> Yes, I've built the `clang` itself and `compiler-rt` project. Then I've
> checked them to run 'check-all' (on built clang and compiler-rt). Everything
> works.
Great! Clang should be large enough.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D119792/new/
https://reviews.llvm.org/D119792
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
