mizvekov 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: ---------------- 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 :) 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