[PATCH] D98971: [C++20] [P1825] Fix bugs with implicit-move from variables of reference type

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added inline comments.


================
Comment at: clang/lib/Sema/SemaStmt.cpp:3095
 
-  // ...non-volatile...
-  if (VD->getType().isVolatileQualified())
-    return false;
-
-  // C++20 [class.copy.elision]p3:
-  // ...rvalue reference to a non-volatile...
-  if (VD->getType()->isRValueReferenceType() &&
-      (!(CESK & CES_AllowRValueReferenceType) ||
-       VD->getType().getNonReferenceType().isVolatileQualified()))
+  if (VD->getType()->isObjectType()) {
+    // C++17 [class.copy.elision]p3:
----------------
Quuxplusone wrote:
> mizvekov wrote:
> > aaronpuchert wrote:
> > > mizvekov wrote:
> > > > Quuxplusone wrote:
> > > > > mizvekov wrote:
> > > > > > mizvekov wrote:
> > > > > > > mizvekov wrote:
> > > > > > > > A drive by fix here would be that we already have a VDType in this context, might as well use it even though original for some reason missed it in this part.
> > > > > > > This whole block is also logically equivalent to the much simpler:
> > > > > > > ```
> > > > > > > if (VDType.isReferenceType()) {
> > > > > > >     if (!(CESK & CES_AllowRValueReferenceType) || !VDType.isRValueReferenceType())
> > > > > > >       return false;
> > > > > > >     VDType = VDType.getNonReferenceType()
> > > > > > > }
> > > > > > > if (!VDType.isObjectType() || VDType.isVolatileQualified()) 
> > > > > > >   return false;
> > > > > > > ```
> > > > > > > But you do have to adjust the comments there and adjust the rest to use VDType consistently :)
> > > > > > > Also, I think it might be possible to even remove the `!VDType.isObjectType() || ` part from my suggestion above, because it might be the only option left if it is not a reference anyway.
> > > > > > ```
> > > > > >   bool isObjectType() const {
> > > > > >     // C++ [basic.types]p8:
> > > > > >     //   An object type is a (possibly cv-qualified) type that is not a
> > > > > >     //   function type, not a reference type, and not a void type.
> > > > > >     return !isReferenceType() && !isFunctionType() && !isVoidType();
> > > > > >   }
> > > > > > ```
> > > > > > So yeah I think you can just make my suggestion be:
> > > > > > ```
> > > > > > if (VDType.isReferenceType()) {
> > > > > >     if (!(CESK & CES_AllowRValueReferenceType) || !VDType.isRValueReferenceType())
> > > > > >       return false;
> > > > > >     VDType = VDType.getNonReferenceType()
> > > > > > }
> > > > > > if (VDType.isVolatileQualified()) 
> > > > > >   return false;
> > > > > > ```
> > > > > > 
> > > > > > 
> > > > > Yeah but I //reaally// don't want to
> > > > > (1) change the meaning of `VDType` in the middle of this function (mantra: "one name = one meaning")
> > > > > (2) get "clever". I don't want to have to think about "Are there any types that are neither object types nor reference types?" (What about function types? What about block types? What about, I dunno, bit-fields?) I want the code to be //obviously correct//, and also to casewise match exactly what the standard says. It says object or rvalue reference type — let's write code that expresses that wording //exactly//.
> > > > How about:
> > > > ```
> > > > QualType VDObjType = VDType;
> > > > if (!VDType.isObjectType()) {
> > > >     if (!(CESK & CES_AllowRValueReferenceType) || !VDType.isRValueReferenceType())
> > > >       return false;
> > > >     VDObjType = VDType.getNonReferenceType();
> > > > }
> > > > if (VDObjType .isVolatileQualified()) 
> > > >   return false;
> > > > ```
> > > > And then `s/VDType/VDObjType/` from here on.
> > > > I think this expresses the meaning of the standard clearly.
> > > That seems like a sensible simplification, the proposed code is indeed a bit repetitive. I'd go with the original suggestion plus the new variable:
> > > 
> > > ```
> > > QualType VDNonRefType = VDType;
> > > if (VDType.isReferenceType()) {
> > >     if (!(CESK & CES_AllowRValueReferenceType) || !VDType.isRValueReferenceType())
> > >       return false;
> > >     VDNonRefType = VDType.getNonReferenceType()
> > > }
> > > if (!VDNonRefType.isObjectType() || VDNonRefType.isVolatileQualified()) 
> > >   return false;
> > > ```
> > > 
> > > or whatever name you find appropriate. Actually it's the type of the `DeclRefExpr`, isn't it? So maybe `DREType`?
> > > 
> > > The initialization might be a bit misleading, an alternative would be to not initialize and have an assignment `VDNonRefType = VDType` in the else branch instead.
> > @aaronpuchert Yeah that combination is good to me also, and I liked the name you suggested better. So +1 :)
> > Actually it's the type of the `DeclRefExpr`, isn't it? So maybe `DREType`?
> 
> The fact that we don't know what it is gives me pause, re introducing it. ;) If I were going to introduce a local synonym for `VDType.getNonReferenceType()` on lines 3105-3108, I guess `VDReferencedType` would be the best name for it; but I don't think there's any reason to introduce another name here.
I think it's just the expression type. It should be precisely the type of `E` in `Sema::getCopyElisionCandidate`. (Since moving doesn't even affect the expression type, just its value category, you can either think of the original expression type or the implicitly moved expression type.)

Why the expression type? Well, in the end we're still returning an expression and not a declaration, even if it's a special kind of expression. That's why we want to know if the expression has non-volatile object type. That's how I would look at it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98971