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