[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:
> 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;
}
```


================
Comment at: clang/lib/Sema/Scope.cpp:184-185
+  //    }
+  if (!getEntity())
+    getParent()->NRVO = *NRVO;
 }
----------------
ChuanqiXu wrote:
> There is a similar problem. It looks not right if the NRVO of the parent owns a value already.
Yes, this is intention. You can take a look at the above comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119792