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

[Evgeny Shulgin via Phabricator via cfe-commits]

Izaron added inline comments.


================
Comment at: clang/include/clang/Sema/Scope.h:518
 
   void addNRVOCandidate(VarDecl *VD) {
+    // every candidate except VD is "spoiled" now, remove them from the set
----------------
ChuanqiXu wrote:
> Firstly I am wondering why here doesn't check `NRVO.getInt()` to shortcut directly. But later I found it would change the logic in `::mergeNRVOIntoParent`:
> ```
> void Scope::mergeNRVOIntoParent() {
>   if (VarDecl *Candidate = NRVO.getPointer()) {
>     if (isDeclScope(Candidate))
>       Candidate->setNRVOVariable(true);
>   }
>   ...
> ```
> 
> It would set NRVO for the candidate in NRVO if it is in current scope. With the context of `addNRVOCandidate` here, I could judge that the change would be:
> ```
> X test(bool B) {
>   X x; // before: no nrvo, after: no nrvo (same)
>   if (B)
>     return x;
>   X y; // before: no nrvo, after: nrvo (better)
>   return y; // Now NRVO.getInt()==true and NRVO.getPointer() == y;
> }
> ```
> 
> Yeah, the behavior is still 'right'. `y` should be NRVO in this case. But the implementation smell bad, if `NRVO.getInt()` is true, we shouldn't do any thing. I am not sure if I state my points well. I mean the implementation might be correct, but it is hard to understand, read and maintain. It'd better to make the reader avoid doing mathmatics when reading the codes.
> I am not sure if I state my points well. I mean the implementation might be correct, but it is hard to understand, read and maintain. It'd better to make the reader avoid doing mathmatics when reading the codes.
I agree that it is really hard to understand and needs to be polished. It took long time for me to construct the code that won't break.

I think that one of the sources of the complexity is the `NRVO` variable itself.
If we could change
```
llvm::PointerIntPair<VarDecl *, 1, bool> NRVO;
```
to something like
```
VarDecl *NRVOCandidate;
bool InvalidatesParentNRVOCandidates;
```
And maybe rename `setNoNRVO()` to smth like `invalidateNRVOCandidates` and so on.

What do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119792