ChuanqiXu added inline comments.
================ 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: > > > > 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. > > > Thanks a lot for the careful review! > > > > > > @ChuanqiXu , could you land this patch please? > > > > > > Many thanks to @Izaron for the original implementation. > > Sure. What's your prefer Name and Mail address? > Thanks! > > Roman Rusyaev <rusyaev...@gmail.com> Oh, I forgot you need edit the ReleaseNotes at clang/docs/ReleaseNotes.rst 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