Page MenuHomePhabricator

[ObjC] For type substitution in generics use a regular recursive type visitor.
ClosedPublic

Authored by vsapsai on Jan 22 2019, 6:16 PM.

Details

Diff Detail

Repository
rL LLVM

Event Timeline

vsapsai created this revision.Jan 22 2019, 6:16 PM
clang/lib/AST/Type.cpp
1295 ↗(On Diff #183010)

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.

vsapsai planned changes to this revision.Jan 24 2019, 4:47 PM
vsapsai added inline comments.
clang/lib/AST/Type.cpp
1295 ↗(On Diff #183010)

You are right, great catch. And I have a test case to confirm that.

I've started this refactoring to avoid copy-pasting

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.

vsapsai updated this revision to Diff 183450.Jan 24 2019, 5:17 PM
  • Add a failing test case.

I've added a test case with typedef and think it should be emitting a warning. I.e., the behaviour with typedef should be the same as without it, modulo different pretty-printing in diagnostic. The interesting part is that this test is failing even without my change. I'll look into that and depending on the fix, I'll update this change accordingly.

vsapsai planned changes to this revision.Jan 25 2019, 4:05 PM
vsapsai added inline comments.
clang/lib/AST/Type.cpp
1295 ↗(On Diff #183010)

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

erik.pilkington accepted this revision.Feb 14 2019, 4:14 PM

LGTM too, I agree this should be NFC with the parent commit.

This revision is now accepted and ready to land.Feb 14 2019, 4:14 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 15 2019, 2:14 PM

Thanks for the review, Erik.