[PATCH] D88491: [ASTContext] Use AllowCXX in all merge*Type methods, strip references

[Johannes Doerfert via Phabricator via cfe-commits]

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