jdoerfert added a comment. In D88491#2306099 <https://reviews.llvm.org/D88491#2306099>, @aaron.ballman wrote:
> Do you have test cases for this change? I didn't see any relevant ones in > D88384 <https://reviews.llvm.org/D88384>. In the revert (rG4fc69ab002382675d84f611f22599cb3cb4a0787 <https://reviews.llvm.org/rG4fc69ab002382675d84f611f22599cb3cb4a0787>) I noted a test broke. I went back and it is `clang/test/Headers/nvptx_device_math_complex.cpp` which is already in tree. I'll try to gracefully accept but reject reference types next. That might work. ================ Comment at: clang/lib/AST/ASTContext.cpp:9174 bool ASTContext::typesAreBlockPointerCompatible(QualType LHS, QualType RHS) { return !mergeTypes(LHS, RHS, true).isNull(); } ---------------- aaron.ballman wrote: > It seems possible to call `typesAreBlockPointerCompatible()` in C++ mode > (`ASTContext::areCommonBaseCompatible()` calls `sameObjCTypeArgs()` which > calls `canAssignObjCObjectTypes()` which calls > `ASTContext::typesAreBlockPointerCompatible()`. I'm not certain if the > assertion will actually trigger though, so tests would be appreciated. > > Are there other cases where `mergeType()` can be transitively called in C++ > mode without having already stripped references? `AllowCXX` is only used from OpenMP code and probably named badly. The idea is that we want to allow roughly matching declarations, so far that meant we don't want to be too strict on exception qualifiers. ================ Comment at: clang/lib/AST/ASTContext.cpp:9430-9433 + if (const ReferenceType *lRT = LHS->getAs<ReferenceType>()) + LHS = lRT->getPointeeType(); + if (const ReferenceType *rRT = RHS->getAs<ReferenceType>()) + RHS = rRT->getPointeeType(); ---------------- aaron.ballman wrote: > This will try to merge a `T&` and `T&&` based on `T` alone, is that expected > or should this be caring about lvalue vs rvalue reference types? This is debatable. The standard is all but clear about this and it is not 100% clear to me what is reasonable. As noted in the general comment, we might be able to get all the functionality we need by gracefully handling this case and saying it is not mergable. I'll give that a try. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D88491/new/ https://reviews.llvm.org/D88491 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
