[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:3102
+    // C++20 [class.copy.elision]p3:
+    // ...either a non-volatile object or an rvalue reference to a non-volatile object type...
+    if (!(CESK & CES_AllowRValueReferenceType))
----------------
This is probably over the line limit, maybe try to reflow this as suggested.


================
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:
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98971