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

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu 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
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119792