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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
