[PATCH] D119792: [Clang] [P2025] Analyze only potential scopes for NRVO

[Roman Rusyaev via Phabricator via cfe-commits]

rusyaev-roman 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;
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119792