Quuxplusone marked 3 inline comments as done.
Quuxplusone 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:
----------------
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D98971/new/
https://reviews.llvm.org/D98971
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
