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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
