[PATCH] D57075: [ObjC] For type substitution in generics use a regular recursive type visitor.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai planned changes to this revision.
vsapsai added inline comments.


================
Comment at: clang/lib/AST/Type.cpp:1295
+
+  QualType VisitObjCObjectType(const ObjCObjectType *objType) {
+    if (!objType->isKindOfType())
----------------
vsapsai wrote:
> erik.pilkington wrote:
> > Does this works with type sugar? i.e. previously calling `Ty->getAs<ObjCObjectType>()` would have stripped through `TypedefType`, but its not obvious to me that this traversal is doing the right thing for that case.
> You are right, great catch. And I have a test case to confirm that.
> 
> I've started this refactoring to avoid copy-pasting
> 
> ```lang=c++
>     QualType modifiedType = recurse(T->getModifiedType());
>     if (modifiedType.isNull())
>       return {};
> 
>     QualType equivalentType = recurse(T->getEquivalentType());
>     if (equivalentType.isNull())
>       return {};
> 
>     if (modifiedType.getAsOpaquePtr()
>           == T->getModifiedType().getAsOpaquePtr() &&
>         equivalentType.getAsOpaquePtr()
>           == T->getEquivalentType().getAsOpaquePtr())
>       return QualType(T, 0);
> ```
> 
> and use `BaseType::VisitAttributedType(attrType)` instead. I think it is possible to achieve the previous behaviour with the traditional recursive visitor. But ideas that I have are pretty complicated and I don't think that's the right trade-off.
The test case I've added isn't really testing regressions in this refactoring, it uncovers already broken functionality. After some investigation turns out there is no simple test case that would be passing before this change and failing afterwards. It is so because we were going past type sugar only in `stripObjCKindOfType` which is called from `sameObjCTypeArgs` to compare type args and type args are [canonicalized during construction](https://github.com/llvm/llvm-project/blob/e9cac31dac90aaf0829b7215778fce41edc946f5/clang/lib/AST/ASTContext.cpp#L4457-L4459), so there is no type sugar. We also have `stripObjCKindOfTypeAndQuals` which has different implementation.

My next step is to have a separate fix for `TypedefTypeParam`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57075/new/

https://reviews.llvm.org/D57075