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